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

cmd/go: when testing, pass Stdout to test child process directly #18153

Closed
akyoto opened this issue Dec 2, 2016 · 23 comments
Closed

cmd/go: when testing, pass Stdout to test child process directly #18153

akyoto opened this issue Dec 2, 2016 · 23 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@akyoto
Copy link
Contributor

akyoto commented Dec 2, 2016

What did you do?

Run go test in a project that uses colored terminal output in the test files (e.g. scarlet).

What did you expect to see?

Colored output like in go 1.7. It worked without problems using https://github.com/fatih/color.

What did you see instead?

Colors don't work. Monochrome output.

Does this issue reproduce with the latest release (go1.7.3)?

No it doesn't. Colored output works with 1.7.3 in go test.

System details

go version go1.8beta1 linux/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/eduard/workspace"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
TERM="dumb"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build997840737=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
GOROOT/bin/go version: go version go1.8beta1 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.8beta1 X:framepointer
uname -sr: Linux 4.4.0-47-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.1 LTS
Release:	16.04
Codename:	xenial
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.23-0ubuntu4) stable release version 2.23, by Roland McGrath et al.
gdb --version: GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
@bradfitz
Copy link
Contributor

bradfitz commented Dec 2, 2016

We've never explicitly supported colors or animation or anything like that.

I recommend you file a bug against https://github.com/fatih/color and then let @fatih file a concrete bug here about what we changed, if anything. It might be a bug in the color package.

@akyoto
Copy link
Contributor Author

akyoto commented Dec 2, 2016

@bradfitz fatih/color#48

@mattn
Copy link
Member

mattn commented Dec 2, 2016

do you set color.NoColor = false ?

@akyoto
Copy link
Contributor Author

akyoto commented Dec 2, 2016

@mattn: Setting it explicitly in the test files fixes the issue.

// NoColor defines if the output is colorized or not. It's dynamically set to
// false or true based on the stdout's file descriptor referring to a terminal
// or not. This is a global option and affects all colors. For more control
// over each color block use the methods DisableColor() individually.
var NoColor = !isatty.IsTerminal(os.Stdout.Fd())

Seems like the problem lies here: https://github.com/mattn/go-isatty/blob/master/isatty_linux.go

@bradfitz
Copy link
Contributor

bradfitz commented Dec 2, 2016

Did Go start setting TERM=dumb this cycle? There was another bug about "go bug" reporting TERM=dumb.

@mattn
Copy link
Member

mattn commented Dec 2, 2016

go test handle output stream from testers. So NoColor should be true on tests.

@minux
Copy link
Member

minux commented Dec 2, 2016 via email

@minux minux closed this as completed Dec 2, 2016
@akyoto
Copy link
Contributor Author

akyoto commented Dec 2, 2016

Excuse me, I don't see how IsTerminal returning false on a terminal is considered correct behaviour.
There were no issues in 1.7.3.

Would you like me to file a bug report on https://github.com/mattn/go-isatty instead?

@mattn
Copy link
Member

mattn commented Dec 2, 2016

Please ask on golang-nuts.

@minux
Copy link
Member

minux commented Dec 2, 2016 via email

@fatih
Copy link
Member

fatih commented Dec 2, 2016

It's likely because go test buffers the test output and thus
the direct output file descriptor is no longer a real tty device.

I've tested this with both go1.7.4 and go1.8.beta1. In 1.7.4 the file descriptor inside a test file is a terminal, however in go1.8beta1 it's no longer the case. This is the code piece I've used to test it:

package main

import (
	"fmt"
	"os"
	"testing"

	isatty "github.com/mattn/go-isatty"
)

func TestTerm(t *testing.T) {
	isTerminal := isatty.IsTerminal(os.Stdout.Fd())
	fmt.Printf("isTerminal = %+v\n", isTerminal)
	fmt.Printf("TERM=%q\n", os.Getenv("TERM"))
}

On 1.7.4 it's:

$ go test
isTerminal = true
TERM="xterm-256color"
PASS
ok  	_/Users/fatih/test	0.006s

on 1.8beta1 it's:

$ go test
isTerminal = false
TERM="xterm-256color"
PASS
ok  	_/Users/fatih/test	0.006s

I've also printed the TERM value to see if that changes somehow during the execution, which is not the case (it's printing my own custom value I've set)

I've checked https://beta.golang.org/doc/go1.8 but couldn't find any information about this change. I've tried to search for any changes about this in the codereview mail list and https://github.com/golang/go/blob/master/doc/go1.8.txt but there was no issues that might affect this. Can anyone point me if there is any information I can check why go test buffers or the respective change that triggers this?

@mikioh
Copy link
Contributor

mikioh commented Dec 2, 2016

@fatih,

Try your test with GOGC=off. When it works, the root cause probably may exist around careless system call invocation in the external package and the fix would be to call runtime.KeepAlive.

@fatih
Copy link
Member

fatih commented Dec 2, 2016

The same function, if executed inside a main package prints the following values:

On 1.7.4:

$ go run main.go
isTerminal = true
TERM="xterm-256color"

On 1.8beta1:

$ go run main.go
isTerminal = true
TERM="xterm-256color"

So it's only different when executed with go test

@mikioh I've set GOGC=off but it still prints false on 1.8beta1

@minux
Copy link
Member

minux commented Dec 2, 2016 via email

@minux
Copy link
Member

minux commented Dec 2, 2016 via email

@fatih
Copy link
Member

fatih commented Dec 2, 2016

@minux I think this should be a part of the release document or somehow documented. There might be projects outside that assumes stdout is a terminal. Thanks for the CL link, at least it makes sense now.

@minux minux reopened this Dec 2, 2016
@minux minux changed the title 1.8 beta: Colored terminal output not working in tests anymore doc: mention cmd/go always caches test output now in release note Dec 2, 2016
@minux minux added this to the Go1.8 milestone Dec 2, 2016
@gopherbot
Copy link

CL https://golang.org/cl/33857 mentions this issue.

@bradfitz bradfitz changed the title doc: mention cmd/go always caches test output now in release note doc: mention streaming cmd/go test output is no longer a terminal? Dec 2, 2016
@bradfitz bradfitz added Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation labels Dec 2, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Dec 5, 2016

We're going to revert part of the change so the test process's output is os.Stdout.

@bradfitz bradfitz self-assigned this Dec 6, 2016
@bradfitz bradfitz changed the title doc: mention streaming cmd/go test output is no longer a terminal? cmd/go: when testing, pass Stdout to test child process directly Dec 6, 2016
@gopherbot
Copy link

CL https://golang.org/cl/34010 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/35957 mentions this issue.

gopherbot pushed a commit that referenced this issue Jan 29, 2017
Fixes (skips) the test on Android, where stdout/stderr are not
terminals.

Updates #18153

Change-Id: Ieca65150362a5c423747ad751e00f76f0b890746
Reviewed-on: https://go-review.googlesource.com/35957
Run-TryBot: Elias Naur <elias.naur@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jan 29, 2018
@golang golang unlocked this conversation Oct 9, 2019
@gopherbot
Copy link

Change https://golang.org/cl/200106 mentions this issue: Revert "cmd/go: fail if a test binary exits with no output"

gopherbot pushed a commit that referenced this issue Oct 9, 2019
This reverts CL 184457.

Reason for revert: introduced failures in the regression test for #18153.

Fixes #34791
Updates #29062

Change-Id: I4040965163f809083c023be055e69b1149d6214e
Reviewed-on: https://go-review.googlesource.com/c/go/+/200106
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mvdan
Copy link
Member

mvdan commented Oct 10, 2019

The decision taken here is making it hard to make certain improvements to go test, such as #29062. In #29062 (comment) I argue why I think we should undo this decision and consistently run tests without a real terminal. I might turn that into a proposal if needed.

@mvdan
Copy link
Member

mvdan commented Oct 13, 2019

My counter-argument above is now a proposal, in case anyone has thoughts: #34877

@golang golang locked and limited conversation to collaborators Oct 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants