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

Unify logging in tests #5487

Merged

Conversation

3vilhamster
Copy link
Member

@3vilhamster 3vilhamster commented Dec 18, 2023

What changed?
I've separate test logger into a dedicated package and centralized verbose testing approach.
Switch to zaptest.New that will print out logs only if test are failing to avoid logging on success.
When required, you can always fallback to Development using -v flag.

Why?
It is better to have common approach to logging in tests.
Simplify debugging issues on CI. Now only in the logs you can find logs related to failed test.
Also, save some space. Unit test run logs from 43 mb to 215kb.

How did you test it?
unit tests/integration tests

Potential risks
None

Release notes

Documentation Changes

Copy link
Member

@Shaddoll Shaddoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the massive change.

@3vilhamster 3vilhamster enabled auto-merge (squash) December 18, 2023 19:50
@3vilhamster 3vilhamster merged commit deed4f0 into cadence-workflow:master Dec 18, 2023
16 checks passed
s.Require().NoError(err)
}
logger := loggerimpl.NewLogger(zapLogger)
logger := testlogger.New(s.T())
Copy link
Member

@Groxx Groxx Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately:

panic: Log in goroutine after TestScavengerTestSuite/TestExpiredTasksFollowedByAlive has completed: 2023-12-18T22:18:12.457Z	INFO	Tasklist scavenger stopped	{"logging-call-at": "scavenger.go:201"}
goroutine 243 [running]:
testing.(*common).logDepth(0xc0009d7860, {0xc0001ea2a0, 0x60}, 0x3)
	/usr/local/go/src/testing/testing.go:1003 +0x68c
testing.(*common).log(...)
	/usr/local/go/src/testing/testing.go:985
testing.(*common).Logf(0xc0009d7860, {0x15894f4, 0x2}, {0xc000016040, 0x1, 0x1})
	/usr/local/go/src/testing/testing.go:1036 +0xa5
go.uber.org/zap/zaptest.testingWriter.Write({{0x25e9a20?, 0xc0009d7860?}, 0xa0?}, {0xc0001cc400, 0x61, 0x400})
	/go/pkg/mod/go.uber.org/zap@v1.13.0/zaptest/logger.go:130 +0x12d
go.uber.org/zap/zapcore.(*ioCore).Write(0xc0009fa1e0, {0x0, {0xc1584f291b469190, 0x2bd47304, 0x2d30fe0}, {0x0, 0x0}, {0x15a5897, 0x1a}, {0x0, ...}, ...}, ...)
	/go/pkg/mod/go.uber.org/zap@v1.13.0/zapcore/core.go:90 +0x19a
go.uber.org/zap/zapcore.(*CheckedEntry).Write(0xc00014cdc0, {0xc000c12300, 0x1, 0x1})
	/go/pkg/mod/go.uber.org/zap@v1.13.0/zapcore/entry.go:216 +0x2ed
go.uber.org/zap.(*Logger).Info(0xc000119dc0?, {0x15a5897, 0x1a}, {0xc000c12300, 0x1, 0x1})
	/go/pkg/mod/go.uber.org/zap@v1.13.0/logger.go:187 +0x6b
github.com/uber/cadence/common/log/loggerimpl.(*loggerImpl).Info(0xc000119dc0, {0x15a5897, 0x1a}, {0x0, 0x0, 0x0})
	/cadence/common/log/loggerimpl/logger.go:132 +0xd7
github.com/uber/cadence/service/worker/scanner/tasklist.(*Scavenger).Stop(0xc0005d0b60)
	/cadence/service/worker/scanner/tasklist/scavenger.go:201 +0x19f
created by github.com/uber/cadence/service/worker/scanner/tasklist.(*Scavenger).run.func1
	/cadence/service/worker/scanner/tasklist/scavenger.go:213 +0xaf
FAIL	github.com/uber/cadence/service/worker/scanner/tasklist	0.748s

iirc that's true for a fair number of these, sadly :|

incrementally undoing them as they cause issues seems worthwhile though, and other changes might have improved these since I last tried.

@3vilhamster 3vilhamster deleted the integration-tests-logging branch December 18, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants