Skip to content

testing: use t.Cleanup() for test utilities, remove gotest.tools dependency #21

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 20, 2022
Merged

testing: use t.Cleanup() for test utilities, remove gotest.tools dependency #21

merged 2 commits into from
Nov 20, 2022

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 31, 2020 •
edited
Loading

LOL; cleaning up branches, found this one that I apparently forgot about

@thaJeztah
Copy link
Member Author

oh, hm, interesting; I just realised this was added in Go 1.14

https://github.com/golang/go/blob/go1.14/src/testing/testing.go#L547

so why does this work on go 1.12 and 1.13? 🤔

https://github.com/golang/go/blob/go1.12/src/testing/testing.go#L498
https://github.com/golang/go/blob/go1.13/src/testing/testing.go#L542

@thaJeztah thaJeztah marked this pull request as draft October 31, 2020 14:19
@thaJeztah thaJeztah changed the title testing: use t.Cleanup() for test utilities [hold, investigating] testing: use t.Cleanup() for test utilities Oct 31, 2020
@thaJeztah thaJeztah mentioned this pull request Oct 31, 2020
Merged
@thaJeztah
Copy link
Member Author

rebased, now that #19 was merged; this should now likely fail on Go1.13, but pass on Go1.14 and 1.15

@thaJeztah
Copy link
Member Author

Hmmm.... doesn't fail. I'm confused

@thaJeztah
Copy link
Member Author

What the???? It's showing that it's installing Go 1.13, but it's actually running Go 1.14.10?

panic: CLEANED UP [recovered]
	panic: CLEANED UP

goroutine 33 [running]:
testing.tRunner.func1.1(0x5f04c0, 0x684780)
	/opt/hostedtoolcache/go/1.14.10/x64/src/testing/testing.go:999 +0x319
testing.tRunner.func1(0xc000117320)

@thaJeztah
Copy link
Member Author

Oh!! I know what's wrong; look at GOROOT:

Run actions/checkout@v2
  with:
    repository: moby/term
    token: ***
    ssh-strict: true
    persist-credentials: true
    clean: true
    fetch-depth: 1
    lfs: false
    submodules: false
  env:
    GOROOT: /opt/hostedtoolcache/go/1.13.15/x64

And this is how we run our tests:

run: sudo go test -v ./...

We're using sudo, not sudo -E, so it will likely use the default GOROOT

@thaJeztah
Copy link
Member Author

Getting closer;

# internal/cpu
compile: version "go1.13.15" does not match go tool version "go1.14.10"
# runtime/internal/atomic
compile: version "go1.13.15" does not match go tool version "go1.14.10"

@thaJeztah
Copy link
Member Author

Looks like PATH doesn't match the right version of Golang;

sudo -E sh -c 'go version && go env && go test -v ./...'
  shell: /bin/bash -e {0}
  env:
    GOROOT: /opt/hostedtoolcache/go/1.13.15/x64
go version go1.14.10 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/runner/.cache/go-build"
GOENV="/home/runner/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/runner/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/hostedtoolcache/go/1.13.15/x64"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/hostedtoolcache/go/1.13.15/x64/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/runner/work/term/term/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build834888489=/tmp/go-build -gno-record-gcc-switches"
go: downloading golang.org/x/sys v0.0.0-20200831180312-196b9ba8737a
go: downloading gotest.tools/v3 v3.0.2
go: downloading github.com/creack/pty v1.1.11
go: downloading github.com/google/go-cmp v0.4.0
go: downloading github.com/pkg/errors v0.9.1
# internal/cpu
compile: version "go1.13.15" does not match go tool version "go1.14.10"

@thaJeztah
Copy link
Member Author

Looks to be the PATH indeed; It's because sudo (as a security measure) does not inherit PATH https://unix.stackexchange.com/a/83194

here's results of running without and with sudo -E (with some cleaning up; sorted all env-vars, and removed build-specific random values)

4_Nosudo.txt
6_Sudo -E.txt

Without sudo, the PATH is

PATH=/home/runner/go/bin:/opt/hostedtoolcache/go/1.13.15/x64/bin:/home/linuxbrew/.linuxbrew/bin:/home/linuxbrew/.linuxbrew/sbin:/opt/pipx_bin:/usr/share/rust/.cargo/bin:/home/runner/.config/composer/vendor/bin:/home/runner/.dotnet/tools:/snap/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin

Whereas with sudo -E, path is the default:

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin

@thaJeztah
Copy link
Member Author

Success! It now properly fails with Go 1.13, but succeeds on Go 1.14 and Go 1.15 (as expected);

Error: ./term_test.go:21:4: t.Cleanup undefined (type *testing.T has no field or method Cleanup)
Error: ./term_test.go:35:4: t.Cleanup undefined (type *testing.T has no field or method Cleanup)

@thaJeztah thaJeztah mentioned this pull request Nov 1, 2020
Merged
@thaJeztah thaJeztah changed the title [hold, investigating] testing: use t.Cleanup() for test utilities [hold until we drop Go 1.13] testing: use t.Cleanup() for test utilities Nov 1, 2020
@thaJeztah thaJeztah force-pushed the simplify_util branch 2 times, most recently from a9d63ce to 3c61562 Compare November 1, 2020 16:51
This was referenced Nov 5, 2020
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Feb 17, 2021
Merged
@thaJeztah thaJeztah mentioned this pull request Jul 20, 2022
Merged
@thaJeztah thaJeztah force-pushed the simplify_util branch 3 times, most recently from e955174 to fd17c6f Compare November 5, 2022 15:22
@thaJeztah thaJeztah mentioned this pull request Nov 5, 2022
Closed
@thaJeztah thaJeztah changed the title [hold until we drop Go 1.13] testing: use t.Cleanup() for test utilities testing: use t.Cleanup() for test utilities, remove gotest.tools dependency Nov 5, 2022
This was referenced Nov 5, 2022
Merged
Merged
Draft
@thaJeztah
testing: use t.Cleanup() for test utilities
5feae0b 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
remove gotest.tools for testing
360619c 
Reduce the number of dependencies for this module.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review November 5, 2022 22:14
@thaJeztah
Copy link
Member Author

Two years in the making; looks like it's ready for prime-time now 🎉

@dims @cpuguy83 PTAL

dims
dims approved these changes Nov 6, 2022
View reviewed changes
Copy link
Collaborator

@dims dims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! nice clean ups in go.mod/sum

@thaJeztah
Copy link
Member Author

Let me bring this one one 👍

@thaJeztah thaJeztah merged commit abb1982 into moby:master Nov 20, 2022
@thaJeztah thaJeztah deleted the simplify_util branch November 20, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims dims approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thaJeztah @dims