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

go/cmd: test output changes with -bench=., -fuzz=., etc. #57671

Closed
smoyer64 opened this issue Jan 7, 2023 · 2 comments
Closed

go/cmd: test output changes with -bench=., -fuzz=., etc. #57671

smoyer64 opened this issue Jan 7, 2023 · 2 comments

Comments

@smoyer64
Copy link

smoyer64 commented Jan 7, 2023

What version of Go are you using (go version)?

go version go1.19.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/smoyer1/.cache/go-build"
GOENV="/home/smoyer1/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/smoyer1/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/smoyer1/go:/usr/share/go-tools"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/smoyer1/.asdf/installs/golang/1.19.4/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/smoyer1/.asdf/installs/golang/1.19.4/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/smoyer1/git/git-bug/go.mod"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2844214839=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I don't think I'd classify this as a bug but in my opinion, it does represent undesirable behavior. Running the following test under different conditions result in different outcomes, which may represent a difference in the actual characters emitted to the output:

import (
	"os"
	"testing"

	"github.com/fatih/color"
	"github.com/mattn/go-isatty"
	"golang.org/x/sys/unix"
)

func TestGetTermios(t *testing.T) {
	t.Log("Stdout FD:", os.Stdout.Fd())
	termios, err := unix.IoctlGetTermios(int(os.Stdout.Fd()), unix.TCGETS)
	t.Log("Termios:", termios)
	t.Log("Termios error:", err != nil)
	t.Log("Is a TTY:", isatty.IsTerminal(os.Stdout.Fd()))
	t.Log("Supports color:", !color.NoColor)
}

What did you expect to see?

The same output to stdout regardless of the command that ran the test.

What did you see instead?

Assuming this test is placed in a sub-package, the outcome of this test is as follows:

  • (PASS): go test -v -bench=. ./...
  • (PASS): cd pkg/output && go test -v
  • (FAIL): 'go test -v ./...`
  • (FAIL): go test -v ./pkg/output

This behavior is caused by the code at

var buf bytes.Buffer
if len(pkgArgs) == 0 || testBench != "" || testFuzz != "" {
// Stream test output (no buffering) when no package has
// been given on the command line (implicit current directory)
// or when benchmarking or fuzzing.
// No change to stdout.
} else {
// If we're only running a single package under test or if parallelism is
// set to 1, and if we're displaying all output (testShowPass), we can
// hurry the output along, echoing it as soon as it comes in.
// We still have to copy to &buf for caching the result. This special
// case was introduced in Go 1.5 and is intentionally undocumented:
// the exact details of output buffering are up to the go command and
// subject to change. It would be nice to remove this special case
// entirely, but it is surely very helpful to see progress being made
// when tests are run on slow single-CPU ARM systems.
//
// If we're showing JSON output, then display output as soon as
// possible even when multiple tests are being run: the JSON output
// events are attributed to specific package tests, so interlacing them
// is OK.
if testShowPass() && (len(pkgs) == 1 || cfg.BuildP == 1) || testJSON {
// Write both to stdout and buf, for possible saving
// to cache, and for looking for the "no tests to run" message.
stdout = io.MultiWriter(stdout, &buf)
} else {
stdout = &buf
}
}
and as noted in the comment, has been around longer since at least Go 1.5 (since the special case was added at that point). I'm not arguing that there's a need for the buffering in some cases and that it's safe to write directly to stdout in other cases but rather that the test environment should remain consistent regardless of how the test is launched.

Since there's the need for buffering in some cases, it's impossible for stdout to always be a *unix.Termios. I'd argue that (during tests), stdout should therefore NEVER be a *unix.Termios.

@seankhliao
Copy link
Member

Duplicate of #34877

@seankhliao seankhliao marked this as a duplicate of #34877 Jan 7, 2023
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Jan 7, 2023
@smoyer64
Copy link
Author

smoyer64 commented Jan 7, 2023

I swear I searched high and low for an existing issue that described this problem - thanks for linking/triaging my issue!

@golang golang locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants