Decomposition of integration testsuite - proof of concept

[bartoszkubicki]

Description (*)

All idea comes from this issue. @ihor-sviziev I took apart of Magento_Rss, Magento_AdminNotification also, as there is test here which contains also file data provider. Generally, it was very easy to move the test and locally all tests worked the same way. I don't see any problem in moving, and refactoring tests in similar way (optimizing imports, some return types, types hints)

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Decomposition of dev/tests testsuites across corresponding modules #28012

Manual testing scenarios (*)

1. Just run moved integration tests. If everything still works and tests pass it means that they were successfully decomposed from testsuite

Questions or comments

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 are green)

[m2-assistant]

Hi @bartoszkubicki. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[orlangur]

Just run magento integration tests. If everything still works and tests pass it means that they were successfully decomposed from testsuite

Small note: make sure decomposed tests where actually executed. You don't need to run all tests actually, just moved ones, CI will check the rest.

[orlangur]

LGTM except I don't think we need to add visual noise into tests with : void.

Also, adding declare strict types should be a separate effort.

In pull requests dedicated for integration tests modularity let's just move the tests. Can you do it all over the code at once with some reusable automation?

[ihor-sviziev]

LGTM except I don't think we need to add visual noise into tests with : void.

Also, adding declare strict types should be a separate effort.

In pull requests dedicated for integration tests modularity let's just move the tests. Can you do it all over the code at once with some reusable automation?

That’s why I wanted to wait until php7.4 & phpunit 9 changes will be delivered to 2.4-develop branch, probably they should contains all these strict types and return void.

From my perspective it’s better to add return types and declare strict types and review all tests only once rather then fixing, reviewing, testing all tests few times, but would be great to have separate commits for separate changes if possible

[bartoszkubicki]

@ihor-sviziev is there any problem between php7.4 & phpunit 9 and strct types?
I don't know if it is easier to to these tasks iteratively - moving tests and fixing paths to fixtures AND later adding all types-like stuff.

For sure there is no problem in dividing in particular PRs changes into commits like 1. moving tests 2. fixtures fixing 3. slight refactor + types

If all changes can be backward incompatible (for 2.4) I would say maybe it is better to take a chance of bigger changes.

I would not bundle all work for all module into one PR, because it may be more difficult to review. Some smaller modules may be delivered together, but for example Magneto_Catalog should be done separately. However, if it is possible to do it in some automated way, then it can be done in one batch.

[ihor-sviziev]

@slavvka could you bring results of integration test failure? We can't see any failures in build report

[VladimirZaets]

@ihor-sviziev @bartoszkubicki , I reruned tests cause looks like something went wrong, we also haven't test results in our internal infrastructure

[ihor-sviziev]

@VladimirZaets the same result, integration tests fails.
Maybe some change on infra side should be changed in order to support integration tests in module directories

[VladimirZaets]

@ihor-sviziev @bartoszkubicki testReferencesFromStaticFiles test with different data set is failed.

Due to LESS library specifics, the '../images/arrows-bg.svg' cannot be tested.
#0 /var/www/html/dev/tests/integration/testsuite/Magento/Test/Integrity/StaticFilesTest.php(115): PHPUnit\Framework\Assert::markTestSkipped('Due to LESS lib...')
#1 [internal function]: Magento\Test\Integrity\StaticFilesTest->testReferencesFromStaticFiles('adminhtml', 'Magento/backend', '', '', 'css/styles-old....', '/var/www/html/a...')
#2 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestCase.php(1071): ReflectionMethod->invokeArgs(Object(Magento\Test\Integrity\StaticFilesTest), Array)
#3 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestCase.php(939): PHPUnit\Framework\TestCase->runTest()
#4 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php(698): PHPUnit\Framework\TestCase->runBare()
#5 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestCase.php(894): PHPUnit\Framework\TestResult->run(Object(Magento\Test\Integrity\StaticFilesTest))
#6 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(755): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
#7 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(755): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#8 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(755): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#9 /var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(545): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#10 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(195): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, true)
#11 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(148): PHPUnit\TextUI\Command->run(Array, true)
#12 /var/www/html/vendor/phpunit/phpunit/phpunit(53): PHPUnit\TextUI\Command::main()
#13 {main}

I think we should try to run it locally

[bartoszkubicki]

I don't understand - what we should try to run locally

[ihor-sviziev]

@bartoszkubicki as I understood - following integration test fails: Magento\Test\Integrity\StaticFilesTest->testReferencesFromStaticFiles. Please try to run it locally, probably you'll see the same result

[bartoszkubicki]

@ihor-sviziev @VladimirZaets where is this test placed? I nor have it on my branch neither see it on 2.4-develop in github -. https://github.com/magento/magento2/tree/2.4-develop/dev/tests/static/testsuite/Magento/Test/Integrity

[ihor-sviziev]

@ihor-sviziev @VladimirZaets where is this test placed? I nor have it on my branch neither see it on 2.4-develop in github -. https://github.com/magento/magento2/tree/2.4-develop/dev/tests/static/testsuite/Magento/Test/Integrity

Here is https://github.com/magento/magento2/blob/2.4-develop/dev/tests/integration/testsuite/Magento/Test/Integrity/StaticFilesTest.php

[bartoszkubicki]

@ihor-sviziev @VladimirZaets I have this test passed locally

[ihor-sviziev]

Hi @bartoszkubicki,
Now changes from php 7.4 and phpunit 9 were merged. Could you fix conflict with 2.4-develop branch, so we'll see correct test results? Maybe fix for this issue was already delievered in 2.4-develop

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

Hi @bartoszkubicki,
Looks good, but seems like some files in dev/tests/integration were not deleted after merging with 2.4-develop branch, and static tests complains about these files. You can find them here:
https://github.com/magento/magento2/tree/352b9efe5bfaafa444c9f15f0ce13b02df1547b2/dev/tests/integration/testsuite/Magento/AdminNotification

Could you remove them?

[ihor-sviziev]

@bartoszkubicki Also would be good to squash all your changes into single commit and force push, if you don't mind.

[bartoszkubicki]

@ihor-sviziev ok, tests removed. Should I squash all commits including merge commits?

[ihor-sviziev]

@bartoszkubicki yeah would be nice 👍

[ihor-sviziev]

@magento run Static Tests

[ihor-sviziev]

@bartoszkubicki,
Now changes looks really good.
Could you squash all your changes into single commit and I'll approve it?

[bartoszkubicki]

@ihor-sviziev I was trying to do it, but unfortunately you have merged 2.4-develop to my branch, and now I am not able to rebase it (i have hundreds of commits and conflicts). I was trying to work around it, by applying diffs, but with no success. If there is any easy way to do it, please tell me. If not, the easiest way to achieve the same effects is to create separate branch and commit there changes without this mess (I have prepared already, but I can't change branches to be merged in this PR, so I would have to create separate PR. I have checked it - no conflicts there).

[ihor-sviziev]

@bartoszkubicki in this case I think you could do force push OR just create new PR from your new branch and close this one.

[m2-assistant]

Hi @bartoszkubicki, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.