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

Jump to bottom

cmd/dist: failure of test in "Testing packages" phase causes remaining tests to be skipped #12670

Closed
binarycrusader opened this issue Sep 17, 2015 · 4 comments
Closed

cmd/dist: failure of test in "Testing packages" phase causes remaining tests to be skipped #12670

binarycrusader opened this issue Sep 17, 2015 · 4 comments
Labels
FrozenDueToAge
Milestone
Go1.6

Comments

@binarycrusader
Copy link
Contributor

If one of the tests during the "Testing packages" phase of "go tool dist test" fails, remaining test phases are skipped.

For example, on Solaris, due to issue #12115:

$ ./all.bash
...
##### Testing packages.
...
ok      cmd/fix 0.041s
--- FAIL: TestCgoHandlesWlORIGIN (0.47s)
        go_test.go:257: running testgo [build origin]
        go_test.go:276: standard error:
        go_test.go:277: # origin
                ld: fatal: option '-rpath $ORIGIN' is incompatible with building a relocatable object
                collect2: error: ld returned 1 exit status

        go_test.go:286: go [build origin] failed unexpectedly: exit status 2
FAIL
FAIL    cmd/go  23.903s
ok      cmd/gofmt       0.041s
ok      cmd/internal/goobj      0.030s
ok      cmd/internal/obj        0.013s
ok      cmd/internal/obj/x86    0.036s
ok      cmd/newlink     0.020s
ok      cmd/nm  1.643s
ok      cmd/objdump     4.048s
ok      cmd/pack        2.954s
ok      cmd/pprof/internal/profile      0.009s
ok      cmd/vendor/golang.org/x/arch/arm/armasm 0.013s
ok      cmd/vendor/golang.org/x/arch/x86/x86asm 0.189s
ok      cmd/vet 4.023s
2015/09/17 15:32:52 Failed: exit status 1

We see that it exits almost immediately after the "Testing Packages" phase completes.

Once I fixed issue #12115, suddenly, these additional tests were executed:

##### GOMAXPROCS=2 runtime -cpu=1,2,4
ok      runtime 39.300s

##### sync -cpu=10
ok      sync    0.109s

##### ../misc/cgo/stdio

##### ../misc/cgo/life

##### ../misc/cgo/test
scatter = 60b3a0
hello from C
sqrt is: 0
PASS
ok      _/builds/srwalker/golang/go-12115/misc/cgo/test 1.106s
scatter = 552c10
hello from C
sqrt is: 0
PASS
ok      _/builds/srwalker/golang/go-12115/misc/cgo/test 1.074s

##### ../misc/cgo/testgodefs

##### ../misc/cgo/testso

##### ../misc/cgo/testsovar

##### ../misc/cgo/errors

##### ../doc/progs

real        4.1
user       20.4
sys         5.6

##### ../doc/articles/wiki
No differences encountered
No differences encountered
No differences encountered
No differences encountered
PASS

##### ../doc/codewalk

real        1.8
user        1.9
sys         0.3

##### ../test/bench/shootout
fasta
reverse-complement
nbody
binary-tree
binary-tree-freelist
fannkuch
fannkuch-parallel
regex-dna
regex-dna-parallel
spectral-norm
k-nucleotide
k-nucleotide-parallel
mandelbrot
meteor-contest
pidigits
threadring
chameneosredux

real        8.8
user        9.9
sys         1.9

##### ../test/bench/go1
testing: warning: no tests to run
PASS
ok      _/builds/srwalker/golang/go-12115/test/bench/go1        1.620s

##### ../test

##### API check
+pkg debug/dwarf, const ClassUnknown = 0
+pkg debug/dwarf, const ClassUnknown Class
+pkg math/rand, func Read([]uint8) (int, error)
+pkg math/rand, method (*Rand) Read([]uint8) (int, error)
+pkg net, type DNSError struct, IsTemporary bool
+pkg net/http/httptest, method (*ResponseRecorder) WriteString(string) (int, error)
+pkg strconv, func AppendQuoteRuneToGraphic([]uint8, int32) []uint8
+pkg strconv, func AppendQuoteToGraphic([]uint8, string) []uint8
+pkg strconv, func IsGraphic(int32) bool
+pkg strconv, func QuoteRuneToGraphic(int32) string
+pkg strconv, func QuoteToGraphic(string) string
+pkg text/template, method (ExecError) Error() string
+pkg text/template, type ExecError struct
+pkg text/template, type ExecError struct, Err error
+pkg text/template, type ExecError struct, Name string

ALL TESTS PASSED

---
Installed Go for solaris/amd64 in /builds/srwalker/golang/go
Installed commands in /builds/srwalker/golang/go/bin

This seems undesirable, as it means that even a minor failure in the "Testing packages" phase could result in all remaining tests being skipped, allowing bitrot to set in while the failing test is still being resolved.

However, I can see how this might have been intentional in some cases with the logic being that if any of those tests failed, there wasn't any point in running the remaining. Since I was uncertain of the intent, I've opened this bug.

Personally, my desired alternatives, ranked by preference would be:

  1. don't skip remaining tests
  2. skip remaining tests, but add an option to ignore failures and print a message
    saying that option can be used if all should be run
  3. skip remaining tests, but print a warning
@binarycrusader binarycrusader changed the title dist: failure of test in "Testing packages" phase causes remaining tests to be skipped cmd/dist: failure of test in "Testing packages" phase causes remaining tests to be skipped Sep 17, 2015
@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Sep 17, 2015
@minux
Copy link
Member

minux commented Sep 18, 2015 via email

@binarycrusader
Copy link
Contributor Author

If the current behaviour is really intended, or still desirable, then I'd like to request the addition of a warning message on exit to dist, something like:

WARNING: Remaining tests skipped due to package testing failure; use -k to keep executing after errors.

The current situation leaves contributors unfamiliar with the current subtle behaviour in a bad place, and sometimes I would imagine even experienced contributors might forget about it.

I'm happy to make that change myself if it's agreeable, but I feel like something should be done as the current situation can hide errors subtly.

@rsc
Copy link
Contributor

rsc commented Nov 5, 2015

This is working as intended. Once we find something wrong there's no need to continue enumerating things that are wrong. You're not supposed to stay in a broken state like that for long. You're certainly not supposed to send CLs unless all.bash passes. This is not hiding all.bash failures, because it is an all.bash failure.

@rsc rsc closed this as completed Nov 5, 2015
@binarycrusader
Copy link
Contributor Author

@rsc respectfully, I disagree. While it may be obvious to long-time project members that there were other failures, it was not obvious to me, nor apparently anyone else testing on Solaris, since no one noticed the other test failures until I started to get inquisitive about them.

Most of the test suites I've interacted with in the past generally provide a summary at the end to make this clear, for example, they might say "10/100 tests passed" or "remaining tests skipped".

@golang golang locked and limited conversation to collaborators Nov 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
5 participants
@rsc @minux @binarycrusader @ianlancetaylor @gopherbot