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

zapslog: Test with slogtest #1335

Closed
wants to merge 1 commit into from
Closed

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented Aug 23, 2023

Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves #1334

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #1335 (d064fd7) into master (87577d8) will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1335      +/-   ##
==========================================
+ Coverage   98.28%   98.45%   +0.17%     
==========================================
  Files          53       51       -2     
  Lines        3493     3369     -124     
==========================================
- Hits         3433     3317     -116     
+ Misses         50       44       -6     
+ Partials       10        8       -2     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

abhinav added a commit to abhinav/zap that referenced this pull request Sep 1, 2023
Per the slog.Handler contract,
handlers should not log attributes that are equal to the zero value.
This is equivalent to Zap's `zap.Skip()`.

Discovered by uber-go#1335
Refs uber-go#1334
sywhang pushed a commit that referenced this pull request Sep 1, 2023
Per the slog.Handler contract,
handlers should not log attributes that are equal to the zero value.
This is equivalent to Zap's `zap.Skip()`.

Discovered by #1335
Refs #1334
mway pushed a commit that referenced this pull request Oct 5, 2023
Per the comment on `Encoder.EncodeEntry`, any fields that are empty
including fields on the `Entry` type should be omitted. Omit the `Time`
field when we have empty time.
This also aligns with slog.Handler contract.

Refs #1334
Discovered by #1335

---------

Co-authored-by: Abhinav Gupta <[email protected]>
Adds a test for zapslog that verifies its behavior
with slogtest's TestHandler function.
This verifies compliance with the slog logging contract.

Resolves uber-go#1334
abhinav added a commit that referenced this pull request Feb 5, 2024
…oup. (#1408)

This change adds a test based on testing/slogtest
that verifies compliance with the slog handler contract
(a draft of this was available in #1335),
and fixes all resulting issues.

The two remaining issues were:

- `Group("", attrs)` should inline the new fields
  instead of creating a group with an empty name.
  This was fixed with the use of `zap.Inline`.
- Groups without any attributes should not be created.
  That is, `logger.WithGroup("foo").Info("bar")` should not
  create an empty "foo" namespace (`"foo": {}`).
  This was fixed by keeping track of unapplied groups
  and applying them the first time a field is serialized.

Following this change, slogtest passes as expected.

Refs #1333
Resolves #1334, #1401, #1402
Supersedes #1263, #1335

### TESTS

- passed. arukiidou#1
- This also works in Go 1.22

---------

Signed-off-by: junya koyama <[email protected]>
Co-authored-by: Abhinav Gupta <[email protected]>
@abhinav
Copy link
Collaborator Author

abhinav commented Feb 5, 2024

Superseded by #1408

@abhinav abhinav closed this Feb 5, 2024
@abhinav abhinav deleted the slogtest branch February 5, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

zapslog.Handler: Test with slogtest, fix all failures
1 participant