Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Metricbeat][Aerospike Module] Add support for TLS #38126
[Metricbeat][Aerospike Module] Add support for TLS #38126
Changes from 11 commits
b7221a6
7ba6b99
d89729b
b3f3677
972a183
12d0878
59df82f
8965a52
baec16b
0e7291c
4517d20
d5c1279
26dc676
62ebf2a
5635e75
76772c9
7d057a4
95544f3
689c651
4a15b5b
5db38b6
43f4a74
c737807
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
would you mind switching to
Equalf
and providing a description of what didn't match?Right now if this fails in CI we will get an error like
"A" does not equal "B"
. It is a lot easier to debug if we get something likeAerospike policy username is wrong. got: "A" expected: "B"
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.
Hi,
Thank you for the feedback. I don't mind at all ^^. So something like:
Or something like:
??
Should I also modify the existing tests to use Equalf?
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 don't think you need the test name, when it errors out go test will give us that. I prefer the ones that have expected and got (or something like that).
One thing I think that helps is to fail the test and just double check that the error message gives you enough context that you have some idea of where the problem is. For example telling me that the Username is what didn't match, the value we got, and the expected value goes a long way to helping understand where the problem might be.
I think you can just focus on the tests you are adding. That helps limit the scope of the PR.