Allow 'should be able to cancel a snap' test to pass when a cancel successfully occurs (backport #7351) [release/4.10.x] #7362
Conversation
This file contains 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
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
mergify
bot
requested review from
a team,
aruniverse,
paulius-valiunas and
wgoehrig
as code owners
ben-polinsky
approved these changes
Nov 13, 2024
aruniverse
approved these changes
Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This test had the most failures out of any test in the integration tests pipeline in the last 14 days.
Upon investigation, it seems this test is not actually flaky but that this test will always fail anytime a snap is successfully cancellled. I had modified my native code to sleep after starting the snap in order to always cancel in time. It would fail because of this line:
expect(err.message).to.equal("aborted");. The error message would instead be 'Unknown server response code.' Most of the time the snap completes before the cancelSnap request is processed. On the backend an error is thrown with the message 'aborted' but ultimately that made its way into 'handleUnknownResponse' which results in the error message 'Unknown server response code.' The message of aborted could only be accessed if one calledthis.load()on the request,I changed the aborted HTTP error to be 499 which is an error code to indicate that a client has closed the request. And handled that as a special case in the handleUnknownResponse function.
Originally I thought it would make more sense to add 499 to the getStatus function, but then line 344 would no longer return RpcRequestStatus.Unknown and suddenly I was unable to cancel a snap before it was completed. Im guessing something to do with loading the value on line 360. Either way I didn't think it was worth putting more time into because RPC is deprecated and will be replaced with something else soon.
This is an automatic backport of pull request #7351 done by [Mergify](https://mergify.com).