sdk: Remove TestWriter #7559
sdk: Remove TestWriter #7559
Conversation
The go toolchain already handles grouping test output by test. The TestWriter, generally used to send logs through t.Log causes a couple problems. The first can be seen from the implementation. Loggers can outlive the lifetime of the testing.T instance, which causes panics. The second is that it produces log messages which contain the test name twice. This results in log messages that are very long and hard to read. Sending logs to stdout should make it easier to read the test failure messages.
dnephin
added
the
theme/internal-cleanup
|
Has Previously the problem I had was that if anything failed during If this is only true when using |
|
Sending all the logs to We could have the makefile install and run |
|
Personally, I normally just use |
|
I think there is another problem with The failing test is It's possible this is a bug in test2json |
|
I went looking more into this problem, and it seems that in go1.14 they made some changes to support log streaming which made the problem of mixed up test output worse. Output from goroutines is not going to be attributed to the correct test when sent to I'm going to experiment with a solution that buffers the logs for each test and prints them at the end if the test fails. The go tool already buffers this output, so I imagine it should be fine to keep all of the logs in memory for the lifetime of a test. |
Replaces #7559 See golang/go#38458 and golang/go#38382 (comment) Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. The previous solution did not address this problem because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long and contained the test name twice. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly.
Replaces #7559 Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. The previous solution did not address this problem because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long and contained the test name twice. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly. Related: golang/go#38458 (may be fixed in go1.15) golang/go#38382 (comment)
|
Replaced by #7948 |
Replaces #7559 Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. The previous solution did not address this problem because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long and contained the test name twice. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly. Related: golang/go#38458 (may be fixed in go1.15) golang/go#38382 (comment)
Replaces #7559 Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. Attaching log output from background goroutines also cause data races. If the goroutine outlives the test, it will race with the test being marked done. Previously this was noticed as a panic when logging, but with the race detector enabled it is shown as a data race. The previous solution did not address the problem of correct test attribution because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly. Related: golang/go#38458 (may be fixed in go1.15) golang/go#38382 (comment)
|
When upgrading the sdk from v0.5.0 to v0.6.0, commit 51efba2 caused our ginkgo unit tests to fail. We have the following code set up to run before the test suite: var _ = BeforeSuite(func() {
ts, err := testutil.NewTestServerConfigT(nil, nil)
Expect(err).ToNot(HaveOccurred())
consulTestServer = ts
Expect(consulTestServer).ToNot(BeNil())
})This code works with sdk v0.5.0 and causes the following stack trace with sdk v0.6.0: It looks like However This looks like a regression on the public API for the |
|
The README for the sdk says:
I think it is expected that a test helper requires a You should be able to continue to use |
|
Thanks @dnephin. That's completely reasonable. I wasn't aware of the README comment. I'll delay my sdk upgrade to v0.6.0 and wait for ginkgo to update |
Follow up to #7546
I went back and looked at #5344 to better understand the problem that
TestWriterwas solving.I believe
gotestsum, which we use in CI, solves this same problem without having to wrap the logger in the test suite. Theshort,short-verboseandshort-with-failuresformats handle only printing logs for the tests that have failed.The
TestWritersolution seems like it comes with a couple of disadvantages.The first can be seen from the implementation. Loggers can outlive the lifetime of the testing.T instance, which causes panics.
The second is that it produces log messages which contain the test name twice. This results in log messages that are very long and hard to read. (#7546).
sdk/README/mdsuggests that we don't make any promises about backwards compatibility for thesdk, so I removed the unused functions.