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

[Forwardport] Allow usage of config-global.php when running Integration Tests #18201

Merged
merged 5 commits into from
Oct 3, 2018
Merged

[Forwardport] Allow usage of config-global.php when running Integration Tests #18201

merged 5 commits into from
Oct 3, 2018

Conversation

mage2pratik
Copy link
Contributor

@mage2pratik mage2pratik commented Sep 23, 2018

Original Pull Request

#16361

When running Integration Tests (bin/magento dev:tests:run integration), you will need to customize a couple of files to properly setup the testing environment. Part of this procedure is customizing the phpunit.xml which also defines a file TESTS_GLOBAL_CONFIG_FILE (pointing to either etc/config-global.php or etc/config-global.php.dist). However, configuration files defined in this TESTS_GLOBAL_CONFIG_FILE never end up in the sandbox environment. This issue was also encountered in another issue #15196

With this PR, the TESTS_GLOBAL_CONFIG_FILE is actually put to use. The TESTS_GLOBAL_CONFIG_FILE is picked up properly by the existing bootstrap and then passed on to the constructor of the testing application class. Next, the install() method of the application class is run. This is where the bug occurs. The internal variable $this->globalConfigFile is never used. This PR adds the actual usage of this variable.

Fixed Issues

  1. 2.2.4 : Magento 2 integration tests enables all modules #15196: Magento 2 integration tests enables all modules

Manual testing scenarios

  1. First, setup integration tests without this PR. Once everything is properly running, a new sandbox environment is created under dev/tests/integration/tmp/sandbox-*. The etc/config.php file in this sandbox should include the entries from either etc/config-global.php or etc/config-global.php.dist. However, it does not.
  2. Next, apply this patch and rerun the integration tests. The etc/config.local.php file in the sandbox now should include the entries from the etc/config-global.php file. And this is merged together with the etc/config.php, containing all of the modules added through the Magento setup procedure.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @mage2pratik. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me $VERSION instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-3036 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @mage2pratik. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

*/
private function copyGlobalConfigFile()
{
$targetFile = $this->_configDir . '/config.local.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think config.local.php is the correct file.

#27864

Copy link
Contributor

Choose a reason for hiding this comment

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

image
As far as I understand, it's no longer supported since 2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants