Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test fixtures can set up DI config #7434

Merged
merged 1 commit into from Mar 16, 2015
Merged

Test fixtures can set up DI config #7434

merged 1 commit into from Mar 16, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Mar 15, 2015

In order to add tests for #7429 I added a way to provide DI configuration in test fixtures. This DI config is then applied to Piwik while running during the UI tests (like the "test environment" variables that allow to edit the INI config). That's really cool because then we can override any config/service to mock it.

Here is an example where I replace a service with a mock:

class FailUpdateHttpsFixture extends Fixture
{
    public function provideContainerConfig()
    {
        return array(
            'Piwik\Plugins\CoreUpdater\Updater' => \DI\object('Piwik\Plugins\CoreUpdater\Test\Mock\UpdaterMock'),
        );
    }
}

To provide DI config, a fixture has to implement provideContainerConfig() and return an array of config.
#7429 is based on this PR so it should be merged for 2.12 (and before #7429).

@mnapoli mnapoli added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Mar 15, 2015
@mnapoli mnapoli added this to the Piwik 2.12.0 milestone Mar 15, 2015
@mnapoli mnapoli added the Needs Review PRs that need a code review label Mar 15, 2015
@mnapoli mnapoli changed the title Test fixtures di config Test fixtures can set up DI config Mar 15, 2015
@mnapoli mnapoli mentioned this pull request Mar 15, 2015
Merged
2 tasks
diosmosis
diosmosis reviewed Mar 15, 2015
View reviewed changes
tests/lib/screenshot-testing/support/test-environment.js
@@ -153,6 +153,9 @@ TestingEnvironment.prototype.setupFixture = function (fixtureClass, done) {
self.reload();
self.addPluginOnCmdLineToTestEnv();

self.fixtureClass = fixtureClass;
self.save();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be done in Fixture.php around line 240, this would allow for using the DI this way in system/integration tests too.

@diosmosis
Copy link
Member

👍 next step would be to allow this method to be use in IntegrationTestCase/SystemTestCase (and maybe unit tests, not sure how though).

@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 15, 2015

In unit tests (and maybe integration tests) I tend to build the environment in the test (i.e. create the container with the config) because there are no separate PHP processes involved.

In system tests it could indeed be interesting!

@diosmosis
Copy link
Member

Integration tests sometimes use fixtures, so they might need to override DI. Not sure though...

@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 15, 2015

OK! Anyway that will be the same thing as for system test so no problem for that.

@diosmosis
Copy link
Member

Cool :)

@mattab
Copy link
Member

mattab commented Mar 15, 2015

Could you add documentation (even small) about using DI in Screenshot tests in this file: http://developer.piwik.org/guides/tests-ui#manipulating-the-test-environment ?

Besides missing doc, looks good to me!

mnapoli added a commit to matomo-org/developer-documentation that referenced this pull request Mar 16, 2015
@mnapoli
Documentation for matomo-org/matomo#7434 (DI config in test fixtures)
95bbce2
@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 16, 2015

Documentation added: http://developer.piwik.org/guides/tests-ui#manipulating-the-test-environment

@mattab mattab merged commit fa55ac8 into master Mar 16, 2015
@mnapoli mnapoli deleted the test-fixtures-di-config branch March 16, 2015 05:01
@mattab
Copy link
Member

mattab commented Mar 16, 2015

Question: I didn't merge the pull request myself, but it got merged and it says I merged it. Is it because i merged the other PR in #7429 and it automatically merged this one (somehow) ? I don't get this

@mattab
Copy link
Member

mattab commented Mar 16, 2015

Documentation added: http://developer.piwik.org/guides/tests-ui#manipulating-the-test-environment

Looks good!

@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 16, 2015

@mattab yes when the other PR was merged the commit here was too. I didn't think of that but that's actually simpler than merging the PRs in the correct order, good to know for next time ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
2.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mnapoli @diosmosis @mattab