Skip to content

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

Merged
thaJeztah merged 2 commits into
moby:masterfrom
thaJeztah:simplify_util
Nov 20, 2022
Merged

testing: use t.Cleanup() for test utilities, remove gotest.tools dependency#21
thaJeztah merged 2 commits into
moby:masterfrom
thaJeztah:simplify_util

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Oct 31, 2020

Copy link
Copy Markdown
Member

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

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@thaJeztah
thaJeztah marked this pull request as draft October 31, 2020 14:19
@thaJeztahthaJeztah changed the title testing: use t.Cleanup() for test utilities[hold, investigating] testing: use t.Cleanup() for test utilitiesOct 31, 2020
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

Looks to be the PATH indeed; It's because sudo (as a security measure) does not inherit PATHhttps://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
Copy Markdown
MemberAuthor

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)

@thaJeztahthaJeztah 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 utilitiesNov 1, 2020
@thaJeztah
thaJeztahforce-pushed the simplify_util branch 2 times, most recently from a9d63ce to 3c61562CompareNovember 1, 2020 16:51
@thaJeztah
thaJeztahforce-pushed the simplify_util branch 3 times, most recently from e955174 to fd17c6fCompareNovember 5, 2022 15:22
@thaJeztahthaJeztah changed the title [hold until we drop Go 1.13] testing: use t.Cleanup() for test utilitiestesting: use t.Cleanup() for test utilities, remove gotest.tools dependencyNov 5, 2022
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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
Copy Markdown
MemberAuthor

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

@dimsdims left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
MemberAuthor

Let me bring this one one 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@thaJeztah@dims