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

unit testing of SetLogger #288

Closed
pohly opened this issue Feb 7, 2022 · 0 comments · Fixed by #287
Closed

unit testing of SetLogger #288

pohly opened this issue Feb 7, 2022 · 0 comments · Fixed by #287
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@pohly
Copy link

pohly commented Feb 7, 2022

/kind feature

Describe the solution you'd like

#280 broke stack unwinding when using a logr.Logger as logging implementation. This was caught when trying out the modified klog before a release with these tests:

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/logs/json/klog_test.go

Here's an idea how we can prevent such regressions sooner in the future and provide more consistent testing:

  • split out the formatting code in klog.go into a stand-alone Logger that has its own independent state - already partly done for the testing logger that I worked on a while ago
  • copy the klog_test.go into a "klog/tests" package and modify them so that other logger implementations can reuse them by overriding the expected output checks
  • extend the tests to cover various corner cases that Logger's have to support:
    • KObj handling
    • panics in functions provided by values (String, Error, MarshalLog)
    • different value types
    • duplicate key/value pairs
    • etc. - unit tests in the various Logger implementations should provide plenty of ideas
@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants