Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests for go1.18 #75

Closed
maruel opened this issue Mar 19, 2022 · 9 comments
Closed

Fix tests for go1.18 #75

maruel opened this issue Mar 19, 2022 · 9 comments

Comments

@maruel
Copy link
Owner

maruel commented Mar 19, 2022

As of 426572b, go test ./... crashes on go1.18.

@maruel
Copy link
Owner Author

maruel commented Mar 19, 2022

Examples of failing test cases:

go test ./stack -run TestSnapshot_ToHTML
go test ./internal -run TestMainFn
go test ./internal -run 'TestProcess/0-BasePath'
go test ./internal -run TestProcessTwoSnapshots

@MordFustang21 had you ran go test ./... on PR #74?

@maruel
Copy link
Owner Author

maruel commented Mar 19, 2022

https://github.com/maruel/panicparse/runs/5611499179 shows the failure.

With regards to staticcheck, I confirmed that it is fixed but not yet released, i.e. this works: go install honnef.co/go/tools/cmd/staticcheck@23e1086441d24fed9f668ad1cd4374245118b590 but this doesn't: go install honnef.co/go/tools/cmd/staticcheck@latest.

Let's just pin the version in the github actions file in the meantime.

@maruel
Copy link
Owner Author

maruel commented Mar 19, 2022

The command that fails is:

$ go install -gcflags '-l' ./cmd/panic                                                                    
# github.com/maruel/panicparse/v2/cmd/panic      
runtime.morestack: nosplit stack check too deep  
runtime.morestack: nosplit stack overflow        
        792     guaranteed after split check in main.glob..func14<1>                                                               
        800     after main.glob..func14<1> uses -8
        792     on entry to main.recurse2000<1>  
        800     after main.recurse2000<1> uses -8
        792     on entry to main.recurse1999<1>  
        800     after main.recurse1999<1> uses -8
        792     on entry to main.recurse1998<1>  
        800     after main.recurse1998<1> uses -8
(...)
        800     after main.recurse1503<1> uses -8
        792     on entry to main.recurse1502<1>
        800     after main.recurse1502<1> uses -8
        792     on entry to main.recurse1501<1>
        784     on entry to runtime.morestack<0> (nosplit)
        0       after runtime.morestack<0> (nosplit) uses 784

Sounds like a regression in the Go toolchain after all.

@maruel
Copy link
Owner Author

maruel commented Mar 19, 2022

golang/go#49390 confirms that the Go team considers -gcflags '-l' is mostly officially supported.

Action item: bisect the Go toolchain to pinpoint when the regression occurred then file a bug upstream.

@maruel
Copy link
Owner Author

maruel commented Mar 19, 2022

With panicparse checked out at ~/src/panicparse and go.git at ~/src-oth/golang:

$ chmod +x ~/src/bisect.sh
$ cat ~/src/bisect.sh
#!/bin/bash
set -eu
cd ~/src-oth/golang/src
./make.bash &> /dev/null
cd ~/src/panicparse
go build -gcflags '-l' -o /tmp/panic ./cmd/panic

$ cd ~/src/golang

# Quick sanity check:
$ git log --date=short --format="%h %ad %s" go1.18...$(git merge-base go1.18 go1.17)

$ git bisect start go1.18 $(git merge-base go1.18 go1.17)
$ git bisect run ~/src/bisect.sh

After a few minutes, I got golang/go@c991278 as the culprit.

Given its description, I tested a change 1e7f767 at
https://github.com/maruel/panicparse/actions/runs/5612037032

The problem is that go test -covermode=count ./cmd/panic is broken too! So the toolchain has to be fixed, thus we need to file a bug upstream with a reduced test case.

@maruel
Copy link
Owner Author

maruel commented Mar 19, 2022

Filed golang/go#51814 as upstream issue.

@MordFustang21
Copy link
Contributor

Examples of failing test cases:

go test ./stack -run TestSnapshot_ToHTML
go test ./internal -run TestMainFn
go test ./internal -run 'TestProcess/0-BasePath'
go test ./internal -run TestProcessTwoSnapshots

@MordFustang21 had you ran go test ./... on PR #74?

I did run it but I think I may have ran it with 1.17 because I was just testing the parsing of a 1.18 stack trace.

@tommie
Copy link

tommie commented Apr 4, 2022

Per the culprit commit, there's a way to revert back to the old behavior. Would it be a valid workaround to also set -gcflags=all=-G=0 together with -l and -covermode=count|set?

@maruel maruel closed this as completed in 47acf68 Apr 9, 2022
@maruel
Copy link
Owner Author

maruel commented Apr 9, 2022

Released v2.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants