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

PHP Livecodetest testCodeStyle() method does not use whitelist files #11362

Conversation

adrian-martinez-interactiv4
Copy link
Contributor

@adrian-martinez-interactiv4 adrian-martinez-interactiv4 commented Oct 11, 2017

Description

\Magento\Test\Php\LiveCodeTest::testCodeStyle method does not use whitelist files, as it does \Magento\Test\Php\LiveCodeTest::testCodeMess. Because of that, it takes ages to complete when you only want to test code style for a few modified files.

With proposed changes, it will try to find whitelisted files, and if it does not find any, then full whitelist will be checked. By default the behaviour is the same, as long as you have the ability to limit processed files if you create whitelist files from outside.

Fixed Issues (if relevant)

Improvement

Manual testing scenarios

  1. Create (manually, via git diff, via get_github_changes.php script) Magento/Test/_files/changed_files_local.txt, with references to real modified files to be tested.
  2. Run vendor/bin/phpunit --verbose -c dev/tests/static/phpunit-all.xml.dist.

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)

@orlangur orlangur self-assigned this Oct 11, 2017
@orlangur
Copy link
Contributor

It was done intentionally in #8685 and should not be ever changed in order to avoid regression of fixed problem. PHPCS run takes only few minutes while PHPMD would take hours.

Behavior of $changedFiles = self::getChangedFilesList($changedFilesBaseDir); can be slightly changed in your particular project if current one does not work well.

@orlangur
Copy link
Contributor

Current implementation needs to be changed so that:

  • 100% whitelisted files check is preserved on any CI
  • it is possible to run a reduced check locally

@@ -30,6 +30,7 @@
</testsuites>
<php>
<ini name="date.timezone" value="America/Los_Angeles"/>
<env name="phpunit.static.testcodestyle.use_whitelist" value="0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest changing it to testcodestyle.is_full_scan and default 1 value.

I'm not sure whether it's better to use env or const, like we do in integration tests <const name="TESTS_CLEANUP" value="enabled"/>.

@@ -201,11 +201,16 @@ private function getFullWhitelist()

public function testCodeStyle()
{
$useWhiteList = isset($_ENV['phpunit.static.testcodestyle.use_whitelist'])
&& $_ENV['phpunit.static.testcodestyle.use_whitelist'] === '1';
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply $whitelist = !empty(...is_full_scan...) ? ...

Please use both php and phtml.

@adrian-martinez-interactiv4
Copy link
Contributor Author

adrian-martinez-interactiv4 commented Oct 12, 2017

@vrann I updated this PR to include new const also in phpunit-all.xml.dist file, and add comments, could you check the commit associated to this PR? 205c2f9

The one added in MAGETWO-81646 - bf50355 does not include the last changes, and due to this, conflicts have raised: https://github.com/magento/magento2/pull/11362/conflicts.

@orlangur
Copy link
Contributor

@adrian-martinez-interactiv4 it cannot be reverted, please apply these additional changes on top of new develop and force push to the same branch.

@adrian-martinez-interactiv4
Copy link
Contributor Author

@orlangur Done

@orlangur
Copy link
Contributor

@adrian-martinez-interactiv4

Magento\Test\Php\LiveCodeTest::testCodeStyle
PHPUnit\Framework\Exception: Warning: file_get_contents(.../magento2/dev/tests/static/report/phpcs_report.txt): failed to open stream: No such file or directory in .../magento2/dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php:212.

...1/magento2/dev/tests/static/framework/bootstrap.php:56
.../magento2/dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php:212

Somehow it is not passing on Bamboo currently, I need some time to find out why. No action required from your side, just some patience :)

magento-team pushed a commit that referenced this pull request Oct 16, 2017
@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32
@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@ihor-sviziev
Copy link
Contributor

@orlangur it looks like after f59cfe2 issue should gone away. Could you re-check?

@orlangur
Copy link
Contributor

@ihor-sviziev it is just a workaround, I need to find out how such changes broke the Bamboo CICD loop.

@okorshenko okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018
@okorshenko okorshenko self-assigned this Jan 8, 2018
@magento-engcom-team magento-engcom-team merged commit 22fd782 into magento:2.3-develop Feb 3, 2018
magento-engcom-team pushed a commit that referenced this pull request Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants