-
Notifications
You must be signed in to change notification settings - Fork 699
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
base: main
Are you sure you want to change the base?
Conversation
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. |
c53a440
to
1aeebc7
Compare
be5788a
to
45b54fe
Compare
45b54fe
to
796983b
Compare
Thanks for this and sorry for lag. Example was fixed, but I would love those tests! Do you mind rebasing? |
yep |
796983b
to
70d7244
Compare
rebased, and returned examples function, maybe it's helpful to see how used it. |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2814228
to
02bbafe
Compare
I returned interceptors to test files |
02bbafe
to
6f07a83
Compare
There was a problem hiding this 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.
i work on it |
Changes
Before:
After:
Verification
I added test for testing/verification