ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Enabling h2specd_SUITE.erl again #241

Closed bjosv closed 3 years ago

bjosv commented 3 years ago

The main source file for the test tool h2specd was moved a time ago, so this PR make sure we use correct path when building it. The h2spec project also uses go modules which now is used when building.

The change has been verified (build and passing of h2specd_SUITE) in a Github Action setup that uses a test matrix, consisting of all major versions of Go from v1.11 (2018) to latest 1.15.

bjosv commented 3 years ago

Well, this didn't go as expected.. Running local or using Github Actions seems to give different results for h2specd_SUITE.erl , I'll have to investigate...

Base/Master

Deviating buildkite results in this PR

essen commented 3 years ago

Right there's some BS about Go fetching things and putting them read-only or something. I may or may not have fixed that issue in Cowboy.

bjosv commented 3 years ago

Yeah, the handling of it in Go seems a bit weird too.. the reason for having it read-only is to make sure tests wont write logs amongst source files..

The goto recommendation was to use go clean -modcache to remove files, but in a late version of go (1.14, Feb 2020) they finally added the -modcacherw flag that makes sure the fetched files are read-writable. Adding this flag is probably the correct way of making sure files can be removed easily, but will put tight version restrictions on the build of this test. Any recommendations?

I also checked Cowboy, and if I understand the build-logs correctly the problem seems to be avoided by changing the Buildkite pipeline config, possibly via BUILDKITE_CLEAN_CHECKOUT and BUILDKITE_GIT_CLEAN_FLAGS. I guess I cant change this my self.

essen commented 3 years ago

Went to check. In Cowboy I simply set GOFLAGS=-modcacherw as environment variable. So I guess it's OK to do the same for Gun. And in fact, it was already set for Gun, but not for PR builds (and not for Cowboy PR builds).

Try again? Or do we need to delete the files first?

bjosv commented 3 years ago

I think it will fail due to the initial git clone would fail again. In cowboy this step differs, it does a git remote and git clean first...so that why I thought the pipeline differs regarding BUILDKITE_CLEAN_CHECKOUT or something similar

essen commented 3 years ago

Maybe you're comparing normal Cowboy builds and Gun PR builds. PR instructions are different. I don't set anything special.

I've removed the files, you can try again.

bjosv commented 3 years ago

Ah, I see. Found the environment tab in Buildkit now..

Looks like files are still blocking foralpine and debian, those targets seems to have older Go version anyway (unknown flag) but should get green after removing the old files. Arch Linux is the only target that can build and run the testsuite, but unfortunately some tests in h2specd_SUITE are failing..are there any known issues, or something special with the machine? Like 32bit.. I keep digging in the testsuite..

essen commented 3 years ago

No known issues in that it used to work but nothing's forever. I'll clean up alpine and debian.

Done.

bjosv commented 3 years ago

As an update of this PR: I was able to reproduce one issue and have pushed summerwind/h2spec#119 to fix that problem.

To verify that there are no other issues that I haven't found locally, Ill temporary change the h2spec source to include the correction, and trigger a buildkite run. The plan is to change back to summerwind (with a delivered correction) before delivering this PR. ... Seems that most of the issues are gone in the h2specd run on Arch Linux, but got one new in 1/5 testruns:

6.9.1. The Flow-Control Window

  | × 1: Sends multiple WINDOW_UPDATE frames increasing the flow control window to above 2^31-1   | -> The endpoint MUST sends a GOAWAY frame with a FLOW_CONTROL_ERROR code.   | Expected: GOAWAY Frame (Error Code: FLOW_CONTROL_ERROR)   | Actual: Connection closed

I'll check..

Ubuntu seems to use Go 1.10, but it looks like GOFLAGS was introduced in 1.11, so that target needs some special handing. Go 1.10 does not have go modules either, so it cant build h2spec anyway I guess, hum..

essen commented 3 years ago

Maybe the Ubuntu environment just needs to be updated?

bjosv commented 3 years ago

Ah, yes, updating the Ubuntu env. is probably the best way forward. (Go version in Alpine, Centos, Debian could also be lifted to >=1.14 to get them running the testcase)

essen commented 3 years ago

I have updated the Ubuntu environment to 20.04 which has Go 1.13. Please retry.

bjosv commented 3 years ago

Super, kicked of a retry.

essen commented 3 years ago

Since upstream is taking such a long time perhaps we could do this PR in 2 parts: first part to fix the running issue itself, and second part about the intermittent issue. I could merge the first part immediately.

bjosv commented 3 years ago

Sounds like a plan, I changed so that we now run the h2specd from its source, and hopefully the intermittent issue is not that frequent in CI tests.

essen commented 3 years ago

Merged, thanks!