-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add integration tests for supported event stream operations #676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, consider adding event stream support to protocol-test-helpers
"response": { | ||
"Ok": { | ||
"status": 200, | ||
"version": "HTTP/1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting! Looks like we recorded an HTTP/1.1 session with transfer encoding: chunked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S3 Select doesn't require HTTP 2 since it's not bidirectional.
|
||
// Validate the requests | ||
replayer | ||
.validate(&["content-type", "content-length"], validate_body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work without timesource pinning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S3 Select isn't a bidirectional stream, so we only get events back in the response. So that means only the initial request is being signed, and we're not validating the signature headers here.
(replayer, output) | ||
} | ||
|
||
fn validate_body(expected_body: &[u8], actual_body: &[u8]) -> Result<(), Box<dyn StdError>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add an event stream verifier to protocol-test-util?
Also, shouldn't this look for more than one message in the body? I think this only validates the first message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it was only validating the first message. Fixed.
As for moving the verifier to a common location, I think that makes sense once we have more than one integration test exercising the event request stream. For now, only the Transcribe test is doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
fn validate_body(expected_body: &[u8], actual_body: &[u8]) -> Result<(), Box<dyn StdError>> { | ||
let expected_wrapper = Message::read_from(expected_body).unwrap(); | ||
let expected_msg = Message::read_from(expected_wrapper.payload().as_ref()).unwrap(); | ||
fn decode_frames(mut body: &[u8]) -> Vec<(Message, Option<Message>)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick docstring about what Mesage, Option<Message>
means would be helpful
Motivation and Context
This adds DVR tests for Transcribe's StartStreamTranscription and S3's SelectObjectContent to exercise the full middleware stack for non-RPC event stream operations. This is more work towards #121.
Checklist
I have updated the CHANGELOGNot sure this needs a changelog updateBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.