TestActionsService_DownloadArtifact_invalidLocationHeader fails on Go1.20

[zhsj]

Tested on master version.

$ go version
go version go1.20rc3 linux/amd64

$ go test -run TestActionsService_DownloadArtifact_invalidLocationHeader
--- FAIL: TestActionsService_DownloadArtifact_invalidLocationHeader (0.00s)
    actions_artifacts_test.go:385: Expected URL parse error, got <nil>
FAIL
exit status 1
FAIL    github.com/google/go-github/v49/github  0.008s

[gmlewis]

Thank you, @zhsj . Confirmed.

That seems odd. Whoever wants to take on this issue should read through the go1.20rc3 release notes and/or changelist and see what could be causing this, then find out if it is a bug in the release candidate or a bug in this repo or a breaking change.

[gmlewis]

Just for the record, I get two failures:

$ go test ./...
--- FAIL: TestActionsService_DownloadArtifact_invalidLocationHeader (0.00s)
    actions_artifacts_test.go:385: Expected URL parse error, got <nil>
--- FAIL: TestRepositoriesService_GetArchiveLink_invalidLocationHeader (0.00s)
    repos_contents_test.go:761: Expected URL parse error, got <nil>
FAIL
FAIL	github.com/google/go-github/v49/github	0.360s
FAIL

[gofri]

I updated testURLParseError() as follows:

-       if err, ok := err.(*url.Error); !ok || err.Op != "parse" {
-               t.Errorf("Expected URL parse error, got %+v", err)
+       if nerr, ok := err.(*url.Error); !ok || nerr.Op != "parse" {
+               t.Errorf("Expected URL parse error, got %v %+v (%v->%T)", ok, nerr, err, errors.Unwrap(err))

And got:

--- FAIL: TestActionsService_DownloadArtifact_invalidLocationHeader (0.00s)
    actions_artifacts_test.go:385: Expected URL parse error, got false <nil> (net/http: HTTP/1.x transport connection broken: malformed MIME header line: Location: https://google.com->textproto.ProtocolError)

IMO this test is still helpful (there is a real error), but it needs to handle this new error type.
Also, it might be a good idea to add the printing of the ok value there (and maybe also the original error too; the Unwrap() one is unsafe if ==nil), because the current test output is a bit misleading.
Looks like it's this commit:
golang/go@a6642e6

I'm not really sure what's special about these ctrl-chars in the context of the tests, but I guess that:

• if we really want to test those specifically ==> handle the new error
• if it should test for anything else ==> change the url used for the location

[gmlewis]

Whups, I forgot about this issue when I made #2654 and resolved the issue in #2656.

I dug into these tests, and believe they are redundant. Yes, there is a real error, but it is caught in a different place and the test was written to exercise a specific code path which is impossible to cover with Go 1.20.

I believe this issue can be closed if there are no objections.

[zhsj]

Yeah, the test is removed by #2656