MQE-2229: Deprecation Error When Deprecated ActionGroup References De…

[soumyau]

…precated Element

Description

Fixed Issues (if relevant)

1. magento/magento2-functional-testing-framework#<issue_number>: Issue title
2. ...

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

[coveralls]

Coverage increased (+5.1%) to 61.318% when pulling 55bab02 on MQE-2229 into da3465c on develop.

[jilu1]

• We should add this fix to data and metadata.
  Good catch, fixed.
• There is no unit test before and I remember i ask if unit test is required, i think we decided to add it. Let us know if you have concern.
  Not sure how to write a unit test for a static-check by mocking file system, InputInterface etc. It may not even be possible to write granular unit tests for this command.

[soumyau]

• We should add this fix to data and metadata.
  Good catch, fixed.
• There is no unit test before and I remember i ask if unit test is required, i think we decided to add it. Let us know if you have concern.
  Not sure how to write a unit test for a static-check by mocking file system, InputInterface etc. It may not even be possible to write granular unit tests for this command.

[soumyau]

• We should add this fix to data and metadata.
  Good catch, fixed.
• There is no unit test before and I remember i ask if unit test is required, i think we decided to add it. Let us know if you have concern.
  Not sure how to write a unit test for a static-check by mocking file system, InputInterface etc. It may not even be possible to write granular unit tests for this command.
  @KevinBKozan : https://jira.corp.magento.com/browse/MQE-2229 states to add unit tests. Let us know if if you agree with the above.

[KevinBKozan]

@soumyau yeah unit testing this is a little involved. You'd need to:

• Mock call to $this->scriptUtil->printErrorsToFile() (easy)
• Mock loadAllXmlFiles() to populate all of the $this->xml variables (easy)

Then in the actual function we're testing we have to:

• Abstract file_get_contents() => $this->loadFile() so we can mock it to return xml string + change call in line to $domDocument->loadXML($contents); (easy)
• Mock all required *Handler classes like https://github.com/magento/magento2-functional-testing-framework/blob/MQE-2229/dev/tests/unit/Magento/FunctionalTestFramework/Test/Objects/ActionObjectTest.php#L377 (tricky but simple, need to be well organized to stay sane)
• Doing above then allows scriptUtil to resolve them easily with no mocking required in scriptUtil due to the logic in https://github.com/magento/magento2-functional-testing-framework/blob/MQE-2229/src/Magento/FunctionalTestingFramework/Util/Script/ScriptUtil.php#L280

Personally I think we should add Unit tests, if only because we're not in a time crunch. Also the first class to get unit tests in an area of the framework is always the most arduous, I think it's a solid opportunity to add a lot of coverage to an area of the framework that is pretty barren right now.

If you want to meet in the middle: timebox writing unit tests to 1 day. If it ends up being much trickier than the above we can make stories to add coverage to this part of the FW. Does that sound fair?

[soumyau]

@soumyau yeah unit testing this is a little involved. You'd need to:

• Mock call to $this->scriptUtil->printErrorsToFile() (easy)
• Mock loadAllXmlFiles() to populate all of the $this->xml variables (easy)

Then in the actual function we're testing we have to:

• Abstract file_get_contents() => $this->loadFile() so we can mock it to return xml string + change call in line to $domDocument->loadXML($contents); (easy)
• Mock all required *Handler classes like https://github.com/magento/magento2-functional-testing-framework/blob/MQE-2229/dev/tests/unit/Magento/FunctionalTestFramework/Test/Objects/ActionObjectTest.php#L377 (tricky but simple, need to be well organized to stay sane)
• Doing above then allows scriptUtil to resolve them easily with no mocking required in scriptUtil due to the logic in https://github.com/magento/magento2-functional-testing-framework/blob/MQE-2229/src/Magento/FunctionalTestingFramework/Util/Script/ScriptUtil.php#L280

Personally I think we should add Unit tests, if only because we're not in a time crunch. Also the first class to get unit tests in an area of the framework is always the most arduous, I think it's a solid opportunity to add a lot of coverage to an area of the framework that is pretty barren right now.

If you want to meet in the middle: timebox writing unit tests to 1 day. If it ends up being much trickier than the above we can make stories to add coverage to this part of the FW. Does that sound fair?

@KevinBKozan sure thing. Thanks for the direction. I'll give this a (timeboxed) shot, if the effort is larger, I'll create follow up tickets.

[KevinBKozan]

Small comment on singleton usage; overall works looks solid, just need minor touchup.

[KevinBKozan]

To test loadAllXmlFiles we have two approaches we can take:

• Mock scriptUtil to death
• Introduce a fake module under unit/Resources/DeprecatedUsageModules/ with folder unit/Resources/DeprecatedUsageModules/TestLoadXML/Test/ unit/Resources/DeprecatedUsageModules/TestLoadXML/Suite unit/Resources/DeprecatedUsageModules/TestLoadXML/ActionGroup, etc for each function.

Option two is honestly more of an integration test, but it makes far more sense from a value standpoint than endlessly mocking scriptUtil. A huge part of the value of writing the test is knowing that xml files are actually loaded for use in other functions. So what I think:

• under verification/Tests, introduce DeprecationCheckTest to cover this specific usecase
• create a new TestModule copy like DeprecationCheckModule or DeprecationCheckModules/<specificUseCaseModule>/ with all the correct subfolders to support the testcase so that the test is actually reading real files and testing that functionality.

[jilu1]

Very nice work on the verification test and refactory!

[soumyau]

Small comment on singleton usage; overall works looks solid, just need minor touchup.

Fixed 👍

[soumyau]

Small comment on singleton usage; overall works looks solid, just need minor touchup.

Done.