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

fix zap logger example and add tests #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

theruziev
Copy link

@theruziev theruziev commented May 8, 2023

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Fix zap logging example
  • Add a test for zap example

Before:
Screenshot 2023-05-08 at 16 16 49

After:
Screenshot 2023-05-08 at 16 19 28

Verification

I added test for testing/verification

@google-cla
Copy link

google-cla bot commented May 8, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@theruziev theruziev force-pushed the fix_log_zap_example branch from c53a440 to 1aeebc7 Compare May 8, 2023 10:22
@theruziev theruziev marked this pull request as ready for review May 8, 2023 10:29
@theruziev theruziev changed the title fix zap logger example fix zap logger example and add tests May 8, 2023
@theruziev
Copy link
Author

theruziev commented May 8, 2023

I found discussion (#567) about adding tests for logging examples and I add it.
@bwplotka cc

@theruziev theruziev force-pushed the fix_log_zap_example branch from be5788a to 45b54fe Compare May 8, 2023 15:02
@theruziev theruziev force-pushed the fix_log_zap_example branch from 45b54fe to 796983b Compare May 8, 2023 16:59
@bwplotka
Copy link
Collaborator

Thanks for this and sorry for lag. Example was fixed, but I would love those tests! Do you mind rebasing?

@theruziev
Copy link
Author

yep

@theruziev theruziev force-pushed the fix_log_zap_example branch from 796983b to 70d7244 Compare June 16, 2023 18:12
@theruziev
Copy link
Author

rebased, and returned examples function, maybe it's helpful to see how used it.

@theruziev
Copy link
Author

ping @bwplotka @johanbrandhorst

Copy link
Collaborator

@bwplotka bwplotka 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 this!

Tests are great (just I would move out of suite, but not a blocker).

However we kept InterceptorLogger adapters in test exactly so no one depends / import those. It's only for inspiration and copy. There is even comment suggesting that // This code is simple enough to be copied and not imported.. We have to enforce this otherwise people will start to depend on this module which is not meant to be specially versioned and compatibile (!).

Can we move those back?

// Add any other interceptor you want.
),
)
// Output:
}

type kitExampleTestSuite struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's avoid "suite" patterns, especially for single method test. Why not single unit test?

Copy link
Author

Choose a reason for hiding this comment

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

I used the test suite because that already has a server and client I can easily use for a test. I try to find another way to do that

Copy link
Author

Choose a reason for hiding this comment

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

if you have any idea how to that this without test suite I will try to implement that.

Copy link

Choose a reason for hiding this comment

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

Let's avoid "suite" patterns, especially for single method test. Why not single unit test?

Hello, pardon my curiosity but I wonder why should we avoid suite pattern since it gave us a server and client inside?
It gave us expandable way to add more tests in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Test suite types in Go like this are uncommon and unnecessary. If you need a server in a test, you can just create one. If you want to isolate the logic, create a function. Test suites take OOP beyond what is usually necessary in Go. Just write test functions, skip the suite.

@theruziev theruziev force-pushed the fix_log_zap_example branch from 2814228 to 02bbafe Compare June 24, 2023 08:56
@theruziev
Copy link
Author

I returned interceptors to test files

@theruziev theruziev requested a review from bwplotka June 24, 2023 11:23
@theruziev theruziev force-pushed the fix_log_zap_example branch from 02bbafe to 6f07a83 Compare June 25, 2023 18:24
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Rewrite the "test suite" to one or more separate tests.

@theruziev
Copy link
Author

i work on it

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