test: add test for invalid streamID #6940
Conversation
|
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6940 +/- ##
==========================================
- Coverage 82.62% 82.42% -0.20%
==========================================
Files 296 296
Lines 31432 31461 +29
==========================================
- Hits 25970 25933 -37
- Misses 4422 4469 +47
- Partials 1040 1059 +19
|
|
@dfawley @arvindbr8 can you suggest how could i assert/verify the error? |
|
@AnandInguva - Sure, taking a look |
|
Yeah, let me make some changes soon |
Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>
test/end2end_test.go
Outdated
| got := string(goAwayFrame.DebugData()[:]) | ||
| want := "received an illegal stream id: 2. headers frame: [FrameHeader HEADERS flags=END_STREAM|END_HEADERS stream=2 len=60]" | ||
| if got != want { |
There was a problem hiding this comment.
If you want to be fancy:
if got, want := string(goAwayFrame.DebugData()), "received ..."; got != want {
}
dfawley
changed the title
| // the transport throws an error. | ||
| // | ||
| // [HTTP/2 spec]: https://httpwg.org/specs/rfc7540.html#StreamIdentifiers | ||
| func (s) TestClientInvalidStreamID(t *testing.T) { |
There was a problem hiding this comment.
Since we have this test now, and if we don't already have tests for other invalid stream ID conditions (e.g. a lower ID than the previous, or a stream ID of zero for the first stream), then now might be a good time to add related test cases.
There was a problem hiding this comment.
I will take a look today and add if tests are not present.
There was a problem hiding this comment.
stream ID of zero for the first stream
For this case, the http2/frame itself throws an error when we write frames of streamID 0 using serverTester. So I am guessing we can't do this test end to end and ask for question if we need this test at all?
There was a problem hiding this comment.
It would still be nice to have a test for this, but I'm fine with punting for now if it's difficult to write. Presumably this is a different code path on the server (the framer may error reading the frame), so a test would still be a good idea to have one day.
There was a problem hiding this comment.
Added one more test. PTAL
test/end2end_test.go
Outdated
| if got := string(goAwayFrame.DebugData()); got != want { | ||
| want := "received an illegal stream id: 2." | ||
| if got := string(goAwayFrame.DebugData()); !strings.Contains(got, want) { | ||
| t.Fatalf("Error received: %v, want: %v", got, want) |
There was a problem hiding this comment.
Nit: "want: Contains(%v)" or something to indicate what's happening.
There was a problem hiding this comment.
I see it changed below but not here.
There was a problem hiding this comment.
ah right. changed in the previous test as well. thanks
Fixes: #104
RELEASE NOTES: none