MQE-987: Decouple MFTF from Magento #134
Conversation
|
@lenaorobei please review |
| foreach ($_ENV as $key => $var) { | ||
| defined($key) || define($key, $var); | ||
| } | ||
| } |
|
|
||
| return $exists; | ||
| } | ||
| } |
| } | ||
|
|
||
| if ($output->isVerbose()) { | ||
| $output->writeLn("All Suites Generated"); |
There was a problem hiding this comment.
This message looks like not really "verbose". Suggest outputting messages about all suits and leaving All Suites Generated by default.
| ->setDescription('This command generates all test files and suites based on xml declarations') | ||
| ->addArgument('name', InputArgument::OPTIONAL | InputArgument::IS_ARRAY, 'name(s) of specific tests to generate') | ||
| ->addOption("config", null, InputOption::VALUE_REQUIRED, 'default, singleRun, or parallel', 'default') | ||
| ->addOption("force", null,InputOption::VALUE_NONE, 'force generation of tests regardless of Magento Instance Configuration') |
There was a problem hiding this comment.
Will be good to add shortcuts for options (second parameter in the addOption method).
| SuiteGenerator::getInstance()->generateAllSuites($testManifest); | ||
| $testManifest->generate(); | ||
|
|
||
| print "Generate Tests Command Run" . PHP_EOL; |
There was a problem hiding this comment.
$output->writeLn can be used.
| if ($output->isVerbose()) { | ||
| // build a list of the paths | ||
| $afterState = $this->getFileSystemState($finder); | ||
| $deletedFiles = array_diff($beforeState, $afterState); |
There was a problem hiding this comment.
I have doubts that you need $beforeState and $afterState to get the list of deleted files.
$fileSystem->remove() throws an Exception if the file cannot be deleted, so you can rely on the list of files you are intended to delete.
There was a problem hiding this comment.
I am not sure I understand this comment. From what I understand if a file does not exist and filesystem attempts to remove there is no exception, how can I rely on this list? Additionally I am deleting php files in certain directories, that will not tell me explicit file deletion to output.
There was a problem hiding this comment.
Maybe I'm missing something, but this is how I understood it should work.
protected function execute(InputInterface $input, OutputInterface $output)
{
$fileSystem = new Filesystem();
// delete the files mftf generates during test execution in TESTS_BP
$fileSystem->remove(self::GENERATED_FILES);
if ($output->isVerbose()) {
$output->writeln("Folder with generated files deleted.");
}
$finder = new Finder();
$files = $finder->files()->name('*.php')->in(FW_BP . '/src/Magento/FunctionalTestingFramework/Group/');
foreach ($files as $file) {
$path = $file->getRealPath();
$fileSystem->remove($file);
if ($output->isVerbose()) {
$output->writeln(sprintf('File %s deleted.', $path));
}
}
// delete config files if user specifies a hard reset
$isHardReset = $input->getOption('hard');
if ($isHardReset) {
foreach (self::CONFIGURATION_FILES as $configFile) {
if (file_exists($configFile)) {
$fileSystem->remove($configFile);
if ($output->isVerbose()) {
$output->writeln("Config file $configFile deleted.");
}
}
}
}
$output->writeln('<info>Project cleaned successfully.<info>');
}
There was a problem hiding this comment.
Okay I see what you are saying, I didn't want to explicitly loop the files since the filesystem command will go ahead and delete the files included by the finder object on its own but you're right if we want to have the explicit print we'll have to loop the files ourselves and output.
| $this->setName('generate:tests') | ||
| ->setDescription('This command generates all test files and suites based on xml declarations') | ||
| ->addArgument('name', InputArgument::OPTIONAL | InputArgument::IS_ARRAY, 'name(s) of specific tests to generate') | ||
| ->addOption("config", null, InputOption::VALUE_REQUIRED, 'default, singleRun, or parallel', 'default') |
There was a problem hiding this comment.
Please use single quotes.
| { | ||
| $tests = $input->getArgument('name'); | ||
| $config = $input->getOption('config'); | ||
| $json = $input->getOption('tests'); |
There was a problem hiding this comment.
Suggest validating input parameters here.
For example, you try to decode JSON in the parseTestsConfigJson method and it causes a lot of redundant code is executed. If JSON is invalid, execution should stop immediately.
| $process->setWorkingDirectory(TESTS_BP); | ||
| $process->run( | ||
| function ($type, $buffer) use ($output) { | ||
| $output->write($buffer); |
There was a problem hiding this comment.
writeLn will be more readable.
| */ | ||
| private function getGroupAndSuiteConfiguration(array $groups) | ||
| { | ||
| $testConfiguration['tests'] = []; |
There was a problem hiding this comment.
Seems strange to me that $testConfiguration['tests'] = [] but $testConfiguration['suites'] = null.
There was a problem hiding this comment.
I would like them both to be null, but I cannot merge any relevant tests to a value of null. I could do a check for the "first value" and replace instead of merge but this seems like elegant to me. Additionally I could make this command exclusive to test groups and point users to the run suite command if they attempt to run a group that is in fact a suite.
- add new mftf console commands - include new bootstrap from mftf autoloading - update tests and pathing to be windows compatible
|
Hi @lenaorobei, I believe i have addressed all of your comments. I am going to merge this code in at the End of the Day if you don't have any other concerns. |
MQE-2669: Seprated a run:failed command to generate:failed and run:fa…
Description
Fixed Issues (if relevant)
Contribution checklist