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

Last DI changes, required for tests in some PRO builds #8145

Merged
merged 17 commits into from Jun 20, 2015

Conversation

diosmosis
Copy link
Member

This PR includes:

  1. Ability for environments to stack in StaticContainer. This allows us to embed environments (to some level). When a new Environment is created, instead of replacing the StaticContainer instance, it pushes the instance to an internal stack. There is an Environment::destroy() method that must be explicitly called to remove the entry.
  2. Fix the way Fixture::provideContainerConfig() results are used to override DI. Instead of creating a new Fixture instance, we use the instance in the test case class. This way, the fixture's properties will be set properly in child processes.
  3. Reload test vars before manipulating an environment, so changes to test vars will be seen when creating new environments (ie, create Environment, change vars, create embedded Environment will use new vars instead of the original).
  4. Allow embedded environments to override GlobalSettingsProvider in tests by making TestingEnvironmentManipulator only override the provider if it has to. Previously, it would always create its own GlobalSettingsProvider, so the embedded environment's custom one would be replaced.
  5. Change to the way all provideContainerConfig() results are added. Instead of using array_merge, we add them to an array of definition arrays. This way, fixtures & tests can both use DI\add to the same array, and one won't overwrite the other. Also allows doing the same sort of thing w/ DI\decorate.
  6. Moved FakeCliMulti to its own file so it can be reused.
  7. Some other smaller bug fixes to DI test code. Commit messages have more info.

Going commit by commit to review might be hard for the first two or three commits, but I think is the best way to review.

@diosmosis diosmosis added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. labels Jun 19, 2015
@diosmosis diosmosis self-assigned this Jun 19, 2015
@diosmosis diosmosis added this to the 2.14.0 milestone Jun 19, 2015
@diosmosis diosmosis added the Needs Review PRs that need a code review label Jun 19, 2015
* @param \Piwik\Tests\Framework\TestingEnvironmentVariables|null $testEnvironment Ignored.
* @param bool|false $testCaseClass Ignored.
* @param array $extraPluginsToLoad Ignoerd.
*/
public static function loadAllPlugins(TestingEnvironmentVariables $testEnvironment = null, $testCaseClass = false, $extraPluginsToLoad = array())
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity could you explain why we have the parameters but they are ignored? (rather than removing it) Is it because of BC issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mostly didn't want to break code in plugins. I think ideally, this method should never need to be called... maybe I can deprecate it. Maybe.

@mnapoli
Copy link
Contributor

mnapoli commented Jun 19, 2015

Looks good to merge (needs a rebase). The TestingEnvironmentManipulator and stuffs around it starts to get a little complex, harder to follow everything. Hopefully with time we can simplify it more and more.

@diosmosis
Copy link
Member Author

Ok, I will merge this PR.

The TestingEnvironmentManipulator and stuffs around it starts to get a little complex, harder to follow everything.

Can you let me know what specifically is confusing about it? Would be really useful to have an outside perspective at this point, since I am right now very, very immersed in this code. Is it clearer than the TestingEnvironment.php code that was there before?

diosmosis added 12 commits June 19, 2015 17:07
…onment variables, by reload vars in manipulator before manipulating an environment.
…der ONLY if were custom paths used. This allows other Environment classes to provide their own custom provider, w/o it being overridden by tests. Also when getting DI overrides from a fixture, use the fixture that is configured by the test class, so any fixture properties will be correctly initialized.
…ot use array_merge, instead just add them to the array of definition arrays.
…::loadAllPlugins. For users that want to use the method.
…ngEnvironmentManipulator so TestConfig just gets in the way.
…er instead of always returning the list of plugins in getActivatedPlugins(). This makes sure tests that activate/deactive plugins will affect config in memory. Otherwise they won't have the complete effect.
diosmosis added 5 commits June 19, 2015 23:50
…case class and fixture class show up properly.
…xtraDefinitions in SystemTestCase since they are applied by TestingEnvironmentManipulator.
…t case class in TestingEnvironmentManipulator, and add hacky fix for issue where Config will reload after construction, resetting list of activated plugins, after it is modified by the manipulator.
diosmosis added a commit that referenced this pull request Jun 20, 2015
Last DI changes, required for tests in some PRO builds. Includes many small changes + tweaks to test system, w/ regard to DI.
@diosmosis diosmosis merged commit 3969f06 into master Jun 20, 2015
@diosmosis diosmosis deleted the di_allow_embedding_env branch June 20, 2015 09:26
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jun 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants