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

[testing] Switch to logging with zap #3557

Merged
merged 22 commits into from
Nov 21, 2024
Merged

[testing] Switch to logging with zap #3557

merged 22 commits into from
Nov 21, 2024

Conversation

marun
Copy link
Contributor

@marun marun commented Nov 20, 2024

Why this should be merged

Previously testing used an incoherent mix of TestContext.Outf, fmt.Print and fmt.Fprint. This PR switches everything to use the zap logger for consistency with the rest of avalanchego.

How this works

  • switches fixture like tmpnet and the e2e helpers to zap
  • updates all tests to use zap

How this was tested

CI

TODO

  • Figure out why the logger returned by NewSimpleLogger is outputting weird level e.g. Level(-6) instead of INFO

Need to be documented in RELEASES.md?

N/A

@marun marun added the testing This primarily focuses on testing label Nov 20, 2024
@marun marun self-assigned this Nov 20, 2024
@marun marun marked this pull request as ready for review November 20, 2024 20:38
tests/log.go Outdated Show resolved Hide resolved
tests/antithesis/avalanchego/main.go Outdated Show resolved Hide resolved
tests/antithesis/avalanchego/main.go Outdated Show resolved Hide resolved
tests/antithesis/xsvm/main.go Outdated Show resolved Hide resolved
tests/fixture/e2e/helpers.go Outdated Show resolved Hide resolved
tests/fixture/tmpnet/network.go Show resolved Hide resolved
tests/log.go Outdated
Comment on lines 23 to 24
// TODO(marun) Figure out why this is outputting e.g. `Level(-6)` instead of `INFO`
EncodeLevel: zapcore.LowercaseLevelEncoder,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have modified how levels are encoded. We should be providing a custom leveler like in the rest of avalanchego:

func levelEncoder(l zapcore.Level, enc zapcore.PrimitiveArrayEncoder) {
	enc.AppendString(Level(l).String())
}

func jsonLevelEncoder(l zapcore.Level, enc zapcore.PrimitiveArrayEncoder) {
	enc.AppendString(Level(l).LowerString())
}

See utils/logging/format.go#newTermEncoderConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any objection to exporting these functions so they can be reused?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope - makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/log.go Outdated Show resolved Hide resolved
tests/simple_test_context.go Show resolved Hide resolved
marun and others added 3 commits November 21, 2024 20:24
@marun marun force-pushed the testing-switch-to-zap branch from cf8a4d4 to 292016a Compare November 21, 2024 19:27
tests/antithesis/avalanchego/main.go Outdated Show resolved Hide resolved
tests/antithesis/node_health.go Outdated Show resolved Hide resolved
tests/fixture/tmpnet/network_test.go Outdated Show resolved Hide resolved
utils/logging/format.go Show resolved Hide resolved
@marun marun added this pull request to the merge queue Nov 21, 2024
Merged via the queue into master with commit 5cf58be Nov 21, 2024
23 checks passed
@marun marun deleted the testing-switch-to-zap branch November 21, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing This primarily focuses on testing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants