Don't let an unserializable Allure attachment abort the whole test run (#902) - #928
Open
thai2301 wants to merge 1 commit into
Open
Don't let an unserializable Allure attachment abort the whole test run (#902)#928thai2301 wants to merge 1 commit into
thai2301 wants to merge 1 commit into
Conversation
AllureHelper::addAttachmentToCurrentStep() caught the exception serialize() throws for values containing a Closure, then re-threw using $data->getMessage() instead of the caught $exception. Since $data at that point is still the original, unserialized value, this either surfaces the wrong message or, when $data has no getMessage() method, throws a new, unhandled error that aborts the entire suite instead of the single failing test. addAttachmentToLastStep() had the identical serialize() call with no exception handling at all, so it failed the same way. Both now fall back to a descriptive placeholder string when serialization fails, so a debugging attachment that can't be losslessly serialized no longer takes the rest of the test run down with it.
Author
|
@magento run all tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
AllureHelper::addAttachmentToCurrentStep()catches theExceptionthatserialize()throws when the given data contains aClosure, then re-throws using$data->getMessage()instead of the caught$exception.$dataat that point is still the original, unserialized value (in the reported case, a\Throwablebeing attached to the report), so this either surfaces a misleading message or, for values with nogetMessage()method, throws a new, unhandled error — which aborts the entire test run instead of just the one test that failed.addAttachmentToLastStep()has the identicalserialize()call with no exception handling at all, so it fails the same way for the same root cause.This matches the reported symptom exactly: a single failing test's own exception gets attached to Allure via
TestContextExtension::attachExceptionToAllure()→AllureHelper::addAttachmentToCurrentStep($exception, ...). If that exception's object graph contains a boundClosureanywhere in its trace,serialize()fails, and the broken re-throw takes down the whole suite instead of letting the next test run.Fix
Both methods now fall back to a descriptive placeholder string (
Unable to serialize attachment data of type "...": <original message>) whenserialize()fails, instead of ever re-throwing. A debugging attachment that can't be losslessly serialized should not be able to take the rest of the test run down with it.Fixed Issues (if relevant)
Contribution checklist