Skip to content

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

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

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

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

thaJeztah commented Oct 31, 2020

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

thaJeztah commented Nov 1, 2020

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

thaJeztah commented Nov 1, 2020

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

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 1, 2020

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

thaJeztah commented Nov 1, 2020

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

thaJeztah commented Nov 1, 2020

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

thaJeztah commented Nov 1, 2020

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

thaJeztah commented Nov 1, 2020

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

thaJeztah commented Nov 1, 2020

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 <[email protected]>
@thaJeztah
remove gotest.tools for testing
360619c 
Reduce the number of dependencies for this module.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review November 5, 2022 22:14
@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 5, 2022

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

thaJeztah commented Nov 20, 2022

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