Skip to content
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

Delete unnecessary recover error test #1291

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Delete unnecessary recover error test #1291

merged 1 commit into from
Aug 20, 2024

Conversation

lmars
Copy link
Member

@lmars lmars commented Aug 13, 2024

The test removed in this commit expects that using an invalid recovery key leads to an ERROR protocol message being returned which thus fails the connection, but this isn't part of the spec, we make no guarantee that invalid recovery keys are rejected with an ERROR, and in fact returning a CONNECTED protocol message with the error field indicating the recovery key was invalid is a perfectly good response, which is in fact what we will start to do in the future.

The functionality that an ERROR protocol message is treated as fatal by the SDK and fails the connection is still tested elsewhere, so removing this test doesn't materially reduce the amount of test coverage.

The test removed in this commit expects that using an invalid recovery
key leads to an ERROR protocol message being returned which thus fails
the connection, but this isn't part of the spec, we make no guarantee
that invalid recovery keys are rejected, and in fact returning a
CONNECTED protocol message with the error field indicating the recovery
key was invalid is a perfectly good response, which is in fact what we
will start to do in the future.

The functionality that an ERROR protocol message is treated as fatal
and fails the connection is still tested elsewhere, so rmeoving this
test doesn't materially reduce the amount of test coverage.

Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

Btw, this test is related to spec https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c4. Are we planning to remove spec since we are deleting related integration tests?

@lmars
Copy link
Member Author

lmars commented Aug 20, 2024

Btw, this test is related to spec https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c4. Are we planning to remove spec since we are deleting related integration tests?

For clarity, here's what that spec point says:

Any other ERROR ProtocolMessage indicating a fatal error in the connection. The server will close the transport immediately after. The client should transition to the FAILED state triggering all attached channels to transition to the FAILED state as well. Additionally the Connection#errorReason will be set should be set with the error received from Ably

That is still valid, if the server sends an ERROR message then it should be handled as this spec point describes, but the test deleted in this PR is making a connection with an invalid connection key and expecting that to trigger an ERROR which will no longer happen in the future, it will instead get a CONNECTED message with an error field and thus the test won't pass.

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 20, 2024

Btw, this test is related to spec https://sdk.ably.com/builds/ably/specification/main/features/#RTN15c4. Are we planning to remove spec since we are deleting related integration tests?

For clarity, here's what that spec point says:

Any other ERROR ProtocolMessage indicating a fatal error in the connection. The server will close the transport immediately after. The client should transition to the FAILED state triggering all attached channels to transition to the FAILED state as well. Additionally the Connection#errorReason will be set should be set with the error received from Ably

That is still valid, if the server sends an ERROR message then it should be handled as this spec point describes, but the test deleted in this PR is making a connection with an invalid connection key and expecting that to trigger an ERROR which will no longer happen in the future, it will instead get a CONNECTED message with an error field and thus the test won't pass.

Then can please you suggest some sort of test for the missing spec. I mean I don't want spec implementation without related test

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

lgtm

@lmars
Copy link
Member Author

lmars commented Aug 20, 2024

@sacOO7 yes agreed, I'll follow up with a suggestion but going to merge this to avoid blocking other work, thanks.

@lmars lmars merged commit ad86379 into main Aug 20, 2024
7 of 9 checks passed
@lmars lmars deleted the delete-recover-test branch August 20, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants