Frame size is now 24bit. Decode exception must be adjusted

[mad-p]

Additional frame size fixes.

[coveralls]

Coverage remained the same when pulling 72ff149 on lepidum:frame-size-test-fix into 61db3ee on igrigorik:master.

[igrigorik]

Hmm, are you sure? You're setting flags to 0x0..

• [0,1] = 8+16bits (length)
• [2] = type
• [3] = flags

Or, am I missing something? Also, as an aside, some of these tests could definitely use some refactoring :)

[mad-p]

Hmm. In this code, frame is an octet string. frame[3] is type.

Setting type to zero (DATA) causes an exception
"receiving a DATA frame for unopened stream".

We should explicitly send DATA frame, rather than modify HEADERS octets.

[coveralls]

Coverage remained the same when pulling 0f25817 on lepidum:frame-size-test-fix into 61db3ee on igrigorik:master.

[coveralls]

Coverage remained the same when pulling 2c196cf on lepidum:frame-size-test-fix into 61db3ee on igrigorik:master.

[igrigorik]

Hmm, not sure about this one either. We already have a test for "error if first frame is not SETTINGS":
https://github.com/lepidum/http-2/blob/frame-size-test-fix/spec/connection_spec.rb#L11

The intent of this test was to test a decode exception - i.e. a malformed frame that contains bogus or invalid data.

[mad-p]

I agree.

How can we craft a "malformed frame"?

• SETTINGS with stream_id != 0 (I think this best fits)
• PING, PRIORITY, WINDOW_UPDATE, RST_STREAM with wrong payload size (We have to implement these validations anyway)
• HEADERS with bad opcodes (It depends on HPACK version)

[mad-p]

Hmm.. There seems no validations on incoming frames in framer.rb.
Should we implement those before attacking this test?

I might mock framer.parse to raise an error (instead of feeding crafted octets).

@conn.instance_eval{@framer}.should_receive(:parse).and_raise(CompressionError.new)

[igrigorik]

I think the wording of current test is misleading. We're testing connection-level logic in this set of tests, and framing errors should be handled elsewhere. I propose we:

• Update "should raise connection error on encode exception" > "should raise connection error on send of invalid frame"
• Update "should raise connection error on decode exception" > "should raise connection error on receipt of invalid frame"

Within the latter test, we can set SETTINGS with stream ID != 0.

Then, independent of the above, we can add some specs (and logic) for frame decode sanity checking:

• We have some encode tests: https://github.com/lepidum/http-2/blob/frame-size-test-fix/spec/framer_spec.rb#L27-L52 - we'd mirror that for decode.
• We have some encode logic that could be reused for decodes as well: https://github.com/lepidum/http-2/blob/frame-size-test-fix/lib/http/2/framer.rb#L90-L104

Right now we assume that we receive well-formed frames.. which is optimistic. :)

[igrigorik]

bump.. we should probably fix this. @mad-p did my last comment make sense?