Allow 'should be able to cancel a snap' test to pass when a cancel successfully occurs #7351
Conversation
nick4598
requested review from
a team,
aruniverse,
paulius-valiunas and
wgoehrig
as code owners
|
@ben-polinsky pointed out to me that theres a possibility someone is relying on a successfully cancelled snap to produce the error message "Unknown server response code", which is valid. I did consider just modifying the assert in the test to change the error message from I will likely switch over to the simpler fix (changing the assert in the test), as I think breaking someone relying on this behavior is probably a bigger risk. Open to feedback otherwise. Also opening an issue to revisit this test during / after the RPC refactor |
…s-core into nick/fix'flaky'test
|
@Mergifyio backport release/4.10.x release/4.11.x |
✅ Backports have been created
|
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.
