MQE-1884: MFTF - <after> failures override other failures

[soumyau]

attaching suppressed exception before _after hook failure to current step.

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 (+0.3%) to 53.621% when pulling 722f312 on MQE-1884 into 6c13d9d on develop.

[coveralls]

Coverage decreased (-0.006%) to 53.612% when pulling d10be09 on MQE-1884 into 7f8fe40 on develop.

[KevinBKozan]

Left some feedback purely on the implementation side.

I think we need a little more time to test this. In the case of two comparison/errors I think we are good to go:

In the case of two broken steps, I do not think so, only the first broken step is injected into the test.

[KevinBKozan]

I'm not sure what 173~176 do. ExceptionWrapper.php does not have a variable named $exception, and at this point you already have the thing you need to log.

Is this leftover test flow or something like that?

[soumyau]

removed the redundant code.

[KevinBKozan]

On further experimentation, I am not sure I am seeing desired behavior with Codeception 2.4.0+:

[soumyau]

Standalone mftf (codeception 2.3.9( doesn't seem to report broken steps correctly. After upgrading mftf to 2.4.5, standalone mftf reports:

[KevinBKozan]

Question:

• Should we attach every exception, not just the buried one? It feels strange to see a test fail in two steps but only see one Exception attached. We could even label it Before/Test/After Exception using some functions in this class already.

[soumyau]

Those were my initial thoughts too, but looks like After block's exception stack trace appears in the allure report twice - one at the top and another at the bottom after test steps. Will it be a duplication in some way? Capturing context and appending it to attachment name would work, however, there will only be 1 exception in Before + Test body scope, would context be redundant to display? Also looks like Allure doesn't show clear separation of Before/Test/After blocks of the test. What are your thoughts?

[KevinBKozan]

So good news is that with just Codeception 2.4.0 we can do a couple things:

• Stop iterating over errors on line 88, they will only now be parent->previous() relationship
• Given above, we can instead iterate by doing logError($error), wherein we log the parent, check for child and call logError($childError) to pop the full stack
• remove the testFail() hook entirely, just use testEnd() to log these

We can modify the hook to look something like:

...
$testResultObject = call_user_func(\Closure::bind(
    function () use ($cest) {
        return $cest->getTestResultObject();
    },
    $cest
));
if (!empty($testResultObject->errors())) {
    foreach ($testResultObject->errors() as $error) {
        $this->attachExceptionToAllure($error->thrownException());
    }
}
if (!empty($testResultObject->failures())) {
    foreach ($testResultObject->failures() as $error) {
        $this->attachExceptionToAllure($error->thrownException());
    }
}
...

We can end up with something like this

You could add to the signature of attachExceptionToAllure to inlcude a prefix like Error vs Failure as well for clearer deliniation.

[KevinBKozan]

Even if we have some duplication, I think if we lean on prefixing the attachments it would be best in case someone needs the info. Those can be ignored by simply not expanding them.

[soumyau]

Added context as a prefix to the attachment. Including a prefix like Error or Failure may need more investigation. From what I understood, PHPUnit\Framework\Exception are marked as failures and Magento\Framework\Exception are marked as Errors. Maybe there are exceptions which don't follow this criteria.

[KevinBKozan]

I think you are right in not trusting that the exception types are necessarily linked to failure vs error. You could include them in the signature of your new attachExceptionToAllure but I think having the prefix is enough for now.

[KevinBKozan]

I left a comment on the internal ticket, check that out before merging please!