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
Conversation
* @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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
Ok, I will merge this PR.
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? |
…ironments in a single PHP execution.
…to all PHP child processes during tests.
…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.
8f4d748
to
e17d7b7
Compare
…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.
…y, DI test overrides will set it.
… to override in test cases/fixtures.
Last DI changes, required for tests in some PRO builds. Includes many small changes + tweaks to test system, w/ regard to DI.
This PR includes:
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.