Relaxes assertions on query parameters #771
Merged
Conversation
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
jhump
commented
Feb 5, 2024
Comment on lines
+162
to
+164
| errs = append(errs, checkError(expected.Error, actual.Error)...) | ||
| errs = append(errs, checkPayloads(expected.Payloads, actual.Payloads)...) | ||
|
|
There was a problem hiding this comment.
This was a slight UX improvement -- often, the reason a test case fails is because an unexpected error occurs. Previously when that happened, the test case would complain about response headers and trailers before getting to the "meat" of the problem: the error. So this reorders the checks so it is more likely the most relevant errors are reported first.
- No need to assert precise encoding of message parameter.
- Also moves error and payload assertions first since they are
usually most relevant (when present) to a test case failure.
582224a to
c8322ad
Compare
|
cc @rebello95, this should allow you to remove the "known failing" file in connect-swift. This will be in an upcoming v1.0.0-rc3 of the conformance tests (along with some new test cases). |
|
Thanks @jhump! |
smaye81
approved these changes
Feb 7, 2024
smaye81
pushed a commit
that referenced
this pull request
Feb 7, 2024
Previously, computing the expected response would add an expected query parameter for the `message` param, with an encoded value. But the problem is that not all clients would necessarily encode the message in exactly the same way, and that's okay. Some will choose to base64-encode (which is usually more efficient with binary payloads). Also, there's no _canonical_ protobuf binary format, so there could be subtle differences in the byte output, even for equivalent messages. Finally, there's no _canonical_ encoding for compressed payloads -- different default compression settings or other subtle differences in implementation can lead to more than one valid compressed encoding of the same data.
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.
Previously, this would add an expected query parameter for the
messageparam, with an encoded value. But the problem is that not all clients would necessarily encode the message in exactly the same way, and that's fine. Some will choose to base64-encode (which is usually more efficient with binary payloads). Also, there's no canonical protobuf binary format, so there could be subtle differences in the byte output, even for equivalent messages. Finally, there's no canonical encoding for compressed payloads -- different default compression settings or other subtle differences in implementation can lead to more than one valid compressed encoding of the same data.