MQE 1650: Update MFTF configuration to read Test entities from new location #428
Conversation
…cation - update unit tests
…cation - update unit tests
…cation - update unit tests
…cation - update unit tests, verification tests
2 similar comments
jilu1
changed the title
There was a problem hiding this comment.
I was unable to get a test to generate when placed at this path:
magento2ce/dev/tests/acceptance/tests/functional/Magento/FedexTest
Ji and I looked at this together and discussed and I'm still a bit confused. We will discuss tomorrow morning with @okolesnyk.
| } | ||
|
|
||
| if ($foundDeprecate) { | ||
| $deprecaedPath = ltrim( |
| // Suppress print during unit testing | ||
| if (MftfApplicationConfig::getConfig()->getPhase() !== MftfApplicationConfig::UNIT_TEST_PHASE) { | ||
| LoggingUtil::getInstance()->getLogger(ModuleResolver::class)->warning( | ||
| "DEPRECATION: $deprecaedPath is deprecated! Please move mftf test modules to $suggestedPath" |
There was a problem hiding this comment.
Change to:
"DEPRECATION: Found MFTF test modules in the deprecated path: $deprecaedPath. Move these MFTF test modules to $suggestedPath"
The error is confusing without this
| LoggingUtil::getInstance()->getLogger(ModuleResolver::class)->warning( | ||
| "DEPRECATION: $deprecaedPath is deprecated! Please move mftf test modules to $suggestedPath" | ||
| ); | ||
| print ("\nDEPRECATION: $deprecaedPath is deprecated! Please move mftf tests to $suggestedPath\n\n"); |
There was a problem hiding this comment.
turn into a variable and use the same message in both places
also capitalize MFTF in messages to make reading them easier.
| /** | ||
| * Regex to grab vendor name in vendor | ||
| */ | ||
| const VENDOR_NAME_REGEX_V = "~.+\\/" . self::VENDOR . "\/(?<" . self::VENDOR . ">[^\/]+)\/.+~"; |
There was a problem hiding this comment.
Why do we have 3 different regex constants and they are only used in one function in this class.
Can we instead just have one regex to match all 3? What does that look like?
If we can't do that, then at the very least we should improve these variable names to not include single letter acronyms. "_V", "_A", and "_D" are bad naming.
| * | ||
| * @var array|null | ||
| */ | ||
| protected $enabledModulePathsNoFlatten = null; |
There was a problem hiding this comment.
What does flattened mean? Please add to the description of the variable.
| /** | ||
| * Return the modules path based on which modules are enabled in the target Magento instance. | ||
| * | ||
| * @param boolean $flat |
There was a problem hiding this comment.
Please add a short description about the $flat variable to let me know when to use it and when to not use it.
|
Please ensure the original AC includes every path that SHOULD be valid after this change. We should test them all. |
|
Created new PR #445 and will close this one. I will apply some of the comment here into the new PR. |
|
I applied relevant comments into the new PR. Close this one. |
Description
Fixed Issues (if relevant)
Contribution checklist