33308: Eliminated AspectMock usage from ModuleResolverTest.php #852

anzin · 2021-07-07T15:10:34Z

Description

I've eliminated AspectMock usage from dev/tests/unit/Magento/FunctionalTestFramework/Util/ModuleResolverTest.php

Fixed Issues (if relevant)

Fixes [MFTF] Eliminate AspectMock from ModuleResolverTest (Complex!) magento2#33308: [MFTF] Eliminate AspectMock from ModuleResolverTest (Complex!)

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/verification tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)
Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

andrewbess

Hello @anzin
Thank you for your job.

…ock-from-module-resolver-test

jilu1 · 2021-07-22T03:27:17Z

@magento import pull request to https://github.com/magento-commerce/magento2-functional-testing-framework

magento-engcom-team · 2021-07-22T03:28:10Z

@jilu1 the pull request successfully imported.

jilu1 · 2021-07-22T13:26:22Z

@anzin
Thank you for your PR! Please address unit test failure in ModuleResolverTest.

bohdan-harniuk

Hello, @anzin!
Good job! Thank you for your contribution!

Hello, @jilu1!
Could you please check everything here? I've refactored the code a little bit. Now all tests passed successfully!

Thanks, Bohdan

jilu1 · 2021-07-23T13:51:55Z

@magento import pull request to https://github.com/magento-commerce/magento2-functional-testing-framework

magento-engcom-team · 2021-07-23T13:53:11Z

@jilu1 the pull request successfully imported.

jilu1

@anzin @bohdan-harniuk
Can we make sure to have the removed unit tests added back in for the new ModuleResolverService?
For example, testAggregateTestModulePathsDevTests, testGetModulePathsLocations, testGetComposerJsonTestModulePathsForPathInvocation, testGetComposerInstalledTestModulePathsForPathInvocation , etc

anzin · 2021-07-26T17:31:20Z

Hello @jilu1 , sorry I don't understand your question. I removed these tests because we eliminate AspectMock, and these tests are not necessary, or I misunderstood something?

Thank you.

jilu1 · 2021-07-28T04:16:56Z

Hello @jilu1 , sorry I don't understand your question. I removed these tests because we eliminate AspectMock, and these tests are not necessary, or I misunderstood something?

Thank you.

@anzin Can you explain why these tests are not necessary? In other words, should you add these testAggregateTestModulePathsDevTests, testGetModulePathsLocations, testGetComposerJsonTestModulePathsForPathInvocation, testGetComposerInstalledTestModulePathsForPathInvocation as unit test for new class ModuleResolverService?

andrewbess

Hello @anzin
Thank you for your contribution.
Good job.

jilu1 · 2021-08-03T13:44:27Z

dev/tests/unit/Magento/FunctionalTestFramework/Util/ModuleResolverTest.php

+        $moduleResolverService = $this->createPartialMock(ModuleResolverService::class, ['globRelevantPaths']);
+        $moduleResolverService
+            ->method('globRelevantPaths')
+            ->withConsecutive()
+            ->will(
+                $this->returnCallback(
+                    function ($codePath, $pattern) use ($modulePath, $magentoBaseCodePath) {
+                        if ($codePath === $modulePath && $pattern === '') {
+                            return [];
+                        }
+
+                        if ($codePath === $magentoBaseCodePath . '/vendor' && $pattern === 'Test/Mftf') {
+                            return [];
+                        }
+
+                        if ($codePath === $magentoBaseCodePath . "/app/code" && $pattern === 'Test/Mftf') {
+                            return [];
+                        }
+
+                        $this->fail(sprintf(
+                            'Not expected parameter: \'%s\' when invoked method globRelevantPaths().',
+                            $modulePath
+                        ));
+                    }
+                )
+            );


@anzin
This is close but does not match exactly what the original test does. The original test makes sure that all the paths parameters are used at least once, but the new test will miss that.

jilu1

All look good except one comment. Please check.

anzin · 2021-08-04T08:40:41Z

Hello @jilu1 , I've fixed test 'testGetModulePathsLocations', please check if everything is well.

Thanks.

jilu1 · 2021-08-04T15:52:14Z

@magento import pull request to https://github.com/magento-commerce/magento2-functional-testing-framework

magento-engcom-team · 2021-08-04T15:53:10Z

@jilu1 the pull request successfully imported.

bohdan-harniuk

@anzin, thank you for your work here!

…ing-framework into improvement/mftf-33308-eliminate-aspect-mock-from-module-resolver-test

bohdan-harniuk

Hi, @jilu1!

I've actualised and fixed this PR.
Could you please proceed with the code review and check everything?

Thanks, Bohdan

33308: Eliminated AspectMock usage from ModuleResolverTest.php

d218219

andrewbess previously approved these changes Jul 14, 2021

View reviewed changes

andrewbess requested a review from jilu1 July 14, 2021 19:32

Merge branch 'develop' into improvement/mftf-33308-eliminate-aspect-m…

83e1cda

…ock-from-module-resolver-test

bohdan-harniuk added Partner: Atwix partners-contribution Pull Request is created by Magento Partner labels Jul 21, 2021

jilu1 added in progress and removed in progress labels Jul 22, 2021

andrewbess self-assigned this Jul 22, 2021

jilu1 added the needs update label Jul 22, 2021

bohdan-harniuk self-requested a review July 22, 2021 14:55

33308: Code refactoring

c929d3c

bohdan-harniuk dismissed andrewbess’s stale review via c929d3c July 22, 2021 15:04

bohdan-harniuk previously approved these changes Jul 22, 2021

View reviewed changes

jilu1 removed the needs update label Jul 23, 2021

bohdan-harniuk mentioned this pull request Jul 26, 2021

Improvement/mftf 33583 eliminate aspect mock from mock module resolver builder #861

Merged

5 tasks

anzin mentioned this pull request Jul 26, 2021

33582: Eliminated AspectMock from MagentoTestCase.php #863

Merged

5 tasks

jilu1 suggested changes Jul 26, 2021

View reviewed changes

jilu1 added needs update Award: advanced Award: test coverage labels Jul 26, 2021

bohdan-harniuk self-assigned this Jul 27, 2021

anzin requested a review from jilu1 July 30, 2021 20:00

andrewbess previously approved these changes Jul 30, 2021

View reviewed changes

jilu1 reviewed Aug 3, 2021

View reviewed changes

jilu1 suggested changes Aug 3, 2021

View reviewed changes

jilu1 added the needs update label Aug 3, 2021

anzin dismissed andrewbess’s stale review via 5cf7039 August 4, 2021 08:35

anzin force-pushed the improvement/mftf-33308-eliminate-aspect-mock-from-module-resolver-test branch from 5cf7039 to 3a1ae0d Compare August 4, 2021 08:38

anzin requested review from bohdan-harniuk, andrewbess and jilu1 August 4, 2021 08:40

andrewbess previously approved these changes Aug 4, 2021

View reviewed changes

jilu1 removed the needs update label Aug 4, 2021

jilu1 previously approved these changes Aug 4, 2021

View reviewed changes

bohdan-harniuk previously approved these changes Aug 4, 2021

View reviewed changes

bohdan-harniuk added 2 commits August 11, 2021 14:17

Merge branch 'develop' of github.com:magento/magento2-functional-test…

730d8ca

…ing-framework into improvement/mftf-33308-eliminate-aspect-mock-from-module-resolver-test

MFTF-33308: Fixed failed test

89812a2

bohdan-harniuk dismissed stale reviews from jilu1, andrewbess, and themself via 89812a2 August 11, 2021 13:54

bohdan-harniuk force-pushed the improvement/mftf-33308-eliminate-aspect-mock-from-module-resolver-test branch from 3a1ae0d to 89812a2 Compare August 11, 2021 13:54

bohdan-harniuk approved these changes Aug 11, 2021

View reviewed changes

bohdan-harniuk requested review from andrewbess and jilu1 August 11, 2021 14:01

andrewbess approved these changes Aug 11, 2021

View reviewed changes

magento-devops-reposync-svc merged commit a3905a0 into magento:develop Aug 11, 2021

magento-engcom-team removed Award: advanced Award: test coverage labels Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

33308: Eliminated AspectMock usage from ModuleResolverTest.php #852

33308: Eliminated AspectMock usage from ModuleResolverTest.php #852

anzin commented Jul 7, 2021

andrewbess left a comment

jilu1 commented Jul 22, 2021

magento-engcom-team commented Jul 22, 2021

jilu1 commented Jul 22, 2021

bohdan-harniuk left a comment •

edited

Loading

jilu1 commented Jul 23, 2021

magento-engcom-team commented Jul 23, 2021

jilu1 left a comment •

edited

Loading

anzin commented Jul 26, 2021

jilu1 commented Jul 28, 2021 •

edited

Loading

andrewbess left a comment

jilu1 Aug 3, 2021

jilu1 left a comment

anzin commented Aug 4, 2021

jilu1 commented Aug 4, 2021

magento-engcom-team commented Aug 4, 2021

bohdan-harniuk left a comment

bohdan-harniuk left a comment

33308: Eliminated AspectMock usage from ModuleResolverTest.php #852

33308: Eliminated AspectMock usage from ModuleResolverTest.php #852

Conversation

anzin commented Jul 7, 2021

Description

Fixed Issues (if relevant)

Contribution checklist

andrewbess left a comment

Choose a reason for hiding this comment

jilu1 commented Jul 22, 2021

magento-engcom-team commented Jul 22, 2021

jilu1 commented Jul 22, 2021

bohdan-harniuk left a comment • edited Loading

Choose a reason for hiding this comment

jilu1 commented Jul 23, 2021

magento-engcom-team commented Jul 23, 2021

jilu1 left a comment • edited Loading

Choose a reason for hiding this comment

anzin commented Jul 26, 2021

jilu1 commented Jul 28, 2021 • edited Loading

andrewbess left a comment

Choose a reason for hiding this comment

jilu1 Aug 3, 2021

Choose a reason for hiding this comment

jilu1 left a comment

Choose a reason for hiding this comment

anzin commented Aug 4, 2021

jilu1 commented Aug 4, 2021

magento-engcom-team commented Aug 4, 2021

bohdan-harniuk left a comment

Choose a reason for hiding this comment

bohdan-harniuk left a comment

Choose a reason for hiding this comment

bohdan-harniuk left a comment •

edited

Loading

jilu1 left a comment •

edited

Loading

jilu1 commented Jul 28, 2021 •

edited

Loading