Skip to content

MQE-1703: Implicit Suite Generation for Tests #434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 5, 2019
Merged

MQE-1703: Implicit Suite Generation for Tests #434

merged 11 commits into from
Sep 5, 2019

Conversation

soumyau
Copy link
Contributor

@soumyau soumyau commented Aug 28, 2019

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
Copy link

coveralls commented Aug 28, 2019

Coverage Status

Coverage remained the same at 51.974% when pulling 4720af5 on MQE-1703 into 167c7e7 on develop.

Copy link
Contributor

@KevinBKozan KevinBKozan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had several smaller comments I was making as I was seeing code issues, but I condensed it down to one comment. Let me know if you need/want more info on it or want to pair program this, it's not a super straightforward problem.

fixing bug with test generation
Copy link
Contributor

@KevinBKozan KevinBKozan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor code style changes but core functionality is working!

@@ -104,4 +111,27 @@ function ($type, $buffer) use ($output) {
}
return $returnCode;
}

/** Get an array of tests with resolved suite references from $testConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top of docblock needs empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

);
}
// configuration for tests
else $testConfiguration['tests'][] = $test;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to use multiline else formatting:

            } else {
                $testConfiguration['tests'][] = $test;
            }

$testConfiguration['tests'] = null;
$testConfiguration['suites'] = null;
$testsReferencedInSuites = SuiteObjectHandler::getInstance()->getAllTestReferences();
$resolvedTests = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor but rename this to $suiteToTestPair, I thought at first this would hold either type of test but it only holds suite:test pairs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

foreach($tests as $test) {
if (array_key_exists($test, $testsReferencedInSuites)) {
$suites = $testsReferencedInSuites[$test];
$resolvedTests = array_merge(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this array_merge/map combo to something a little more simple?

foreach ($suites as $suite) {
    $resolvedTests[] = "$suite:$test";
}

array_map/merge are both recursive loops at the language level (at least I know map is) so this alternative is either equal or actually faster (easier to read in either case)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was trying to avoid nested loops as I didn't find many implementations of it in code, thought it was a style thing. Changed it.

$testArrayBuilder = [];

foreach ($suitesArray as $suite => $tests) {
$testArrayBuilder = array_merge(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See commend in BaseGenerateCommand, this can probably be simplified into one foreach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

Copy link
Contributor Author

@soumyau soumyau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as per review comments.

$testConfiguration['tests'] = null;
$testConfiguration['suites'] = null;
$testsReferencedInSuites = SuiteObjectHandler::getInstance()->getAllTestReferences();
$resolvedTests = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

@@ -104,4 +111,27 @@ function ($type, $buffer) use ($output) {
}
return $returnCode;
}

/** Get an array of tests with resolved suite references from $testConfiguration
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

$testArrayBuilder = [];

foreach ($suitesArray as $suite => $tests) {
$testArrayBuilder = array_merge(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

foreach($tests as $test) {
if (array_key_exists($test, $testsReferencedInSuites)) {
$suites = $testsReferencedInSuites[$test];
$resolvedTests = array_merge(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was trying to avoid nested loops as I didn't find many implementations of it in code, thought it was a style thing. Changed it.

@soumyau soumyau merged commit a915bd1 into develop Sep 5, 2019
@jilu1 jilu1 deleted the MQE-1703 branch February 6, 2020 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants