[Feature-2188] AuditMessage validation in tests should be more useful / removed #3596
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prabhask5
requested review from
cliu123,
cwperks,
DarshitChanpura,
davidlago,
peternied,
RyanL1997,
stephen-crawford,
reta and
willyborankin
as code owners
prabhask5
changed the title
There was a problem hiding this comment.
Thank you @prabhask5 for this PR. I left some comments around white-space/indentation fixes, please address those and I will re-review them.
src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
Show resolved
Hide resolved
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
This reverts commit 62a9c2c. Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
prabhask5
force-pushed
the
pk-fix-auditvalidation-in-tests
branch
from
429cace to
016759d
Compare
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
|
Not sure how to handle catching the JsonMappingExceptions and making them more specific up the stack trace since apparently the constructor for it is deprecated. Would like some guidance on that if possible |
peternied
reviewed
Oct 27, 2023
src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
Show resolved
Hide resolved
peternied
previously approved these changes
Nov 8, 2023
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
stephen-crawford
previously approved these changes
Nov 10, 2023
src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
Show resolved
Hide resolved
src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
Show resolved
Hide resolved
|
@prabhask5 Please look into the failing CI checks, it looks like there are some outstanding issues that need to be resolved |
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
|
@peternied Most of the errors from before where not related to changes that I made, so I'm pretty confused on how to fix them. I merged main, so hopefully(?) it fixes things. If not, I'd like some guidance on how to proceed. Thanks! |
DarshitChanpura
approved these changes
Nov 14, 2023
peternied
approved these changes
Nov 16, 2023
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Nov 16, 2023
… / removed (#3596) Changed validateMsgs in AbstractAuditlogUnitTest.java to throw exceptions with descriptions instead of just returning true or false. Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu> (cherry picked from commit deff842) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
cwperks
pushed a commit
that referenced
this pull request
Dec 13, 2023
… / removed (#3726) Backport deff842 from #3596. --------- Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Darshit Chanpura <dchanp@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Changed validateMsgs in AbstractAuditlogUnitTest.java to throw exceptions with descriptions instead of just returning true or false. Not sure if the message format level validation in the integration tests is useful yet, so feel free to help me think about the usefullness of that.
Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
Enhancement
Why these changes are required?
Previous version of the tests were unclear in what is erroring, can be hard for developers to debug.
What is the old behavior before changes and new behavior after changes?
validateMsgs and traceback previously returned boolean true or false values without any errors, now they throw Exceptions with specific descriptions of what went wrong.
Issues Resolved
#2188
Is this a backport? If so, please add backport PR # and/or commits #
Testing
Updated tests.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.