MQE-810: Create a static test to validate references between modules #314

KevinBKozan · 2019-02-27T21:44:33Z

Description

Implemented static-checks
Checks the following types of files:
- Test.xml
- Data.xml
- ActionGroup.xml
Performs checks on following reference types:
- xml entity references like{{data.xml}} and {{data.xml(also.parameters)}}
- extends=""
- actionGroup ref=""
Validation is based finding common depdency in file's module and any of the filenames from MFTF entity
- Above means if dataEntity has base or merge in catalog and wishlist, reference will be valid if file has dendepncy on either catalog or wishlist.

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

- Set up ground work for test command

- Set up More ground work for test command

- Added filename to all test objects

- Added filename to page/section

- Implemented Check, only checks first filename found by MFTF.

- fixed actionGroup argument false flagging - reafactored dependencies check to validate on all entity file origins

- Fixed not all directories being parsed - Added check for parameters of elements/urls

- Static/Unit test Fixes

coveralls · 2019-02-28T15:51:42Z

Coverage decreased (-1.5%) to 55.296% when pulling 4851692 on MQE-810-B into 44aecc6 on develop.

jilu1 · 2019-03-25T20:25:29Z

src/Magento/FunctionalTestingFramework/StaticCheck/TestDependencyCheck.php

+
+            if (!empty($violatingReferences)) {
+                // Build error output
+                $errorOutput = "\nFile \"{$filePath->getRealPath()}\"\ncontains references to following modules:\n\t\t";


Should we point out the violation directly? e.g. using words like "SomeTest should not contain references to SomeModule" would be better?

I can change the error output to be more precise, yes.

jilu1 · 2019-03-25T20:30:35Z

src/Magento/FunctionalTestingFramework/StaticCheck/TestDependencyCheck.php

+        }
+        fclose($fileResource);
+        $errorCount = count($errors);
+        $output = "Dependency errors found across {$errorCount} file(s). Error details output to {$outputPath}";


Is it number total tests scanned or number of total errors found? Just look at the error message in console confused me until I see the source code.

Errors unique to file, the $error array is key => value of fileName => [$errors] so the count just counts unique file names.

jilu1

Merge file contents are treated as from the parent module, this produced some false positive errors. For example, AdminAddDefaultVideoBundleProductTest, AdminRemoveDefaultVideoBundleProductTest, etc.

KevinBKozan · 2019-03-26T13:41:13Z

@jilu1 I'm not sure how merge file contents play in here, files are being checked on a per-file basis, not per entity (concept of merging is removed).
I reran my tests and am not finding that the above files are shown as errors. Did you pull down the right branch? It's MQE-810-B, due to the extensive refactor.

jilu1 · 2019-03-26T18:57:56Z

src/Magento/FunctionalTestingFramework/StaticCheck/TestDependencyCheck.php

+        $filePaths = [
+            DIRECTORY_SEPARATOR . 'Test' . DIRECTORY_SEPARATOR,
+            DIRECTORY_SEPARATOR . 'ActionGroup' . DIRECTORY_SEPARATOR,
+            DIRECTORY_SEPARATOR . 'Data' . DIRECTORY_SEPARATOR,


Why not include Page and Section here?

Didn't think it was relevant, mostly. Section can't reference anything other than static selector strings, and while Pages can reference Sections, that linkage is not really used anywhere in MFTF.
If we do build an actual use for Page->Section linkage I think we should add that link check, but otherwise I don't see it as value adding as far as the check goes

jilu1 · 2019-03-26T19:00:53Z

@jilu1 I'm not sure how merge file contents play in here, files are being checked on a per-file basis, not per entity (concept of merging is removed).
I reran my tests and am not finding that the above files are shown as errors. Did you pull down the right branch? It's MQE-810-B, due to the extensive refactor.

You are right. I was using the wrong branch. Please ignore the previous comment

- Updated Output error message

- Fixed Static Check

jilu1 · 2019-03-27T20:19:50Z

src/Magento/FunctionalTestingFramework/StaticCheck/TestDependencyCheck.php

+            // Not a module, is either dev/tests/acceptance or loose folder with test materials
+            if ($moduleFullName == null) {
+                continue;
+            }


I am running standalone mftf, and I have a test module under mftf's dev/tests/functional/tests/MFTF/CatalogTest which has been scanned but it should not.

Will check/resolve this.

jilu1 · 2019-03-27T20:22:33Z

src/Magento/FunctionalTestingFramework/StaticCheck/TestDependencyCheck.php

+     * @param string $subDependencyName
+     * @return array
+     */
+    private function extractSubDependencies($subDependencyName)


This recursive function is fine by itself. But any reason that you don't build the recursion from root level composer file. Will the current way cause duplicate dependencies in the root level?

I think the reason is because it would flatten all the dependencies to one huge array, and we want an array with several module => [dependencies].
We iterate through the big list of dependencies on ln 283 so that we can scope the above flattening.

jilu1 · 2019-03-27T20:30:06Z

src/Magento/FunctionalTestingFramework/StaticCheck/TestDependencyCheck.php

+                    }
+                    // trim `data.field` to `data`
+                    preg_match('/([^.]+)/', $argument, $entityName);
+                    // Double check that {{data.field}} isn't an argument for an ActionGroup


Why skipping? What if it's a direct entity reference in an argument for an ActionGroup? e.g. {{_defatultCategory.name}}

Skipping done on line 175 refers to the following case:

<argument name="product" defaultValue="_defaultProduct"/> ... <comment userInput="{{product.name}} /> <comment userInput="{{_defaultProduct.name}} />

The entity product is just the argument, and if we have a match of name we shouldn't try to resolve it, as the entity doesn't exist.

So in this case the first line would be skipped, but line two would not be.

ok. This is right. I did not look the regex and just thought the argument you tried to skip is in the "value" part.

jilu1

Nice work! I checked a few cases and they all work as expected. Please check the standalone case, and it looks good to me.

jilu1

I could not reproduce the standalone case. Approve this. Things like how to handle existing violations will be done in separate story.

Acqe 2580

KevinBKozan added 8 commits February 26, 2019 08:56

MQE-810: Create a static test to validate references between modules

9c3465d

- Set up ground work for test command

MQE-810: Create a static test to validate references between modules

848bbc7

- Set up More ground work for test command

MQE-810: Create a static test to validate references between modules

a84ed84

- Added filename to all test objects

MQE-810: Create a static test to validate references between modules

61c7600

- Added filename to page/section

MQE-810: Create a static test to validate references between modules

efaaf8d

- Implemented Check, only checks first filename found by MFTF.

MQE-810: Create a static test to validate references between modules

1f10fc4

- fixed actionGroup argument false flagging - reafactored dependencies check to validate on all entity file origins

MQE-810: Create a static test to validate references between modules

56046d6

- Fixed not all directories being parsed - Added check for parameters of elements/urls

MQE-810: Create a static test to validate references between modules

64f82cf

- Static/Unit test Fixes

jilu1 reviewed Mar 25, 2019

View reviewed changes

jilu1 reviewed Mar 26, 2019

View reviewed changes

KevinBKozan added 2 commits March 27, 2019 08:37

MQE-810: Create a static test to validate references between modules

334416c

- Updated Output error message

MQE-810: Create a static test to validate references between modules

e5c9879

- Fixed Static Check

jilu1 reviewed Mar 27, 2019

View reviewed changes

jilu1 suggested changes Mar 28, 2019

View reviewed changes

jilu1 approved these changes Mar 28, 2019

View reviewed changes

Merge branch 'develop' into MQE-810-B

4851692

KevinBKozan merged commit 7f82e4d into develop Mar 29, 2019

KevinBKozan deleted the MQE-810-B branch July 26, 2019 19:28

magento-devops-reposync-svc pushed a commit that referenced this pull request Jul 28, 2023

Merge pull request #314 from magento-gl/ACQE-2580

96e4d9a

Acqe 2580

MQE-810: Create a static test to validate references between modules #314

MQE-810: Create a static test to validate references between modules #314

Uh oh!

Conversation

KevinBKozan commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contribution checklist

Uh oh!

coveralls commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jilu1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinBKozan commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jilu1 commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jilu1 left a comment

Choose a reason for hiding this comment

Uh oh!

jilu1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KevinBKozan commented Feb 27, 2019 •

edited

Loading

coveralls commented Feb 28, 2019 •

edited

Loading

jilu1 left a comment •

edited

Loading