-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add AWSServices discard_regex integration tests #4278
Conversation
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.
After reviewing the proposed test cases I think they fulfill the development made, Therefore it's approved.
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.
Code changes LGTM. However, AWS ITs are available in 4.6.0 so the base branch can be changed. Also, take a look at the failing checks to fix them.
57edfab
to
5f2ffce
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.
LGTM
This PR can be merged once this issue is finished: |
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.
Updating the changelog is required
19a7d65
to
d354572
Compare
d354572
to
522cdcf
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.
I requested some changes but, for me, the most important things to be considered before proceeding with applying those changes are:
-
I see that all test functions do the same and change minimal things like configuration or metadata. I would compress everything in a single test with different cases defined in a single YAML so that the test, according to these cases, knows what to validate and what not. If this is not possible, please detail what and why so that we can discuss about it.
-
I do not see a test case that checks that the JSON field on which you want to filter does not exist (negative case).
-
I don't see a validation to test for not discarding logs that don't apply to the filter set in
<discard_regex>
(negative case).
.../test_aws/data/test_cases/discard_regex_test_module/cases_cloudwatch_discard_regex_json.yaml
Outdated
Show resolved
Hide resolved
...ws/data/test_cases/discard_regex_test_module/cases_cloudwatch_discard_regex_simple_text.yaml
Outdated
Show resolved
Hide resolved
...ration/test_aws/data/test_cases/discard_regex_test_module/cases_inspector_discard_regex.yaml
Outdated
Show resolved
Hide resolved
Hi @mauromalara, regarding your comments:
The single YAML approach was tested, but due to the differences between the base configuration of the module for each case, these failed to execute as expected (e.g., for Cloudwatch having and not having the
The test environment contains both JSON and simple text logs. If the JSON field does not exist, the module does not show any related message, it processes every available log without discarding them.
This is also related to the test environment mentioned previously, if more logs than expected were discarded due to unexpected behavior of the module, then the |
Great! Please, update the issue you mentioned before to explicitly describe that you need to change this to reduce duplicated code.
In my view, I think this might not be good for the user. Picture this situation: as a user, what if I make a mistake while specifying the JSON field? Wouldn't it be preferable to promptly detect such issues by examining the logs? I suggest we create an issue on this matter and, furthermore, develop a test case to encompass this particular scenario.
You're right, didn't think about it. |
b616f48
to
26d67e4
Compare
26d67e4
to
bcc8e01
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.
LGTM!
Test execution 🟢Three consecutive runs of the
|
Description
This PR adds new test cases for the
discard_regex
functionality, more precisely for theCloudWatchLogs
andInspector
services.Added
Tier 0
discard_regex
parameter tests forAWSCloudWatchLogs
discard_regex
parameter tests forAWSInspector
Testing performed
tests/integration/test_aws/test_discard_regex.py