Add ENABLE_PUSH flag in the Upgrade HTTP2-Settings header

[hkwi]

Push flag required for the case the initial upgrade request triggered server push.

[Lukasa]

Thanks for this!

This patch is reasonable, but there are a bunch of tests failing that need to be updated to take account of this change. Do you mind doing that?

[hkwi]

I'll look into.

[Lukasa]

So you've fixed some of the test failures, but you're now not actually testing that this data gets emitted. We do need to add a new test to cover this branch.

[Lukasa]

Rather than break here, can you do this?

http2_settings.settings[SettingsFrame.ENABLE_PUSH] = (
    int(self._enable_push)
)

[hkwi]

Yes.

[Lukasa]

So, as noted, this is not really tested in any sense right now. It would be good to see the appropriate testing changes made.

[hkwi]

One travis-ci job is failing unexpectedly with empty raw log. Can I ask help?

[Lukasa]

I've restarted that build. =)

[hkwi]

I added http11 h2c upgrading test by inheriting TestServerPush which is for http20 h2, and test passed.

[Lukasa]

So, I'm not sure about this approach.

You've added getpushes to the HTTP11Connection object with the goal of having it be a dummy method for catching the upgrade. That's understandable, but I'm not sure that this is the way users should be interacting with the abstract method: in particular because if the connection does not upgrade, which the user cannot guarantee will not happen, the response object is lost.

For the purpose of testing it's probably enough to call getresponse and then check for pushes. How does that sound?

[hkwi]

I agree that HTTP11Connection.getpushes is a little bit hacky. This was added to meet the strict tests of test_promise_before_headers or test_capture_all_promises. If we don't need to satisfy these tests for http11 upgrade, implementation would be more relaxed style. I'll create another PR for that version later.
BTW, the response will be stored in self._response for later self.get_response call, and would not lost.

[Lukasa]

I think those tests don't need to work for upgrade, but we should write a test that does work as expected (e.g. you get a HTTP/2 response and then look for pushes on that stream)