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.

Sign up for GitHub

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

Jump to bottom

feat: adding validation for common recommended fields in sample #866

Closed
wants to merge 8 commits into from
Closed

feat: adding validation for common recommended fields in sample #866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
48db5e8
updating test
harshilgajera-crest Jul 11, 2024
63cbac3
ci: lint changes
harshilgajera-crest Jul 11, 2024
e22193e
ci: updating constants
harshilgajera-crest Jul 11, 2024
9a1cbd8
ci: updating constants
harshilgajera-crest Jul 11, 2024
dc18ac4
ci: updating constants
harshilgajera-crest Jul 11, 2024
021b92e
ci: updating test
harshilgajera-crest Jul 11, 2024
fb44a2a
ci: updating test
harshilgajera-crest Jul 11, 2024
0bc8cf1
ci: updating warning
harshilgajera-crest Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pytest_splunk_addon/cim_tests/test_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,14 @@ def generate_recommended_fields_tests(self):
model = model.replace(" ", "_")
if datasets:
datasets = [dataset.replace(" ", "_") for dataset in datasets]

common_fields = list(
set(event.requirement_test_data["cim_fields"].keys())
& set(event.requirement_test_data["missing_recommended_fields"])
)
if common_fields:
LOGGER.warning(
f"Common fields found in both cim_fields and missing_recommended_fields for {event.sample_name} sample = {common_fields}"
)
Comment on lines +277 to +280
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we log an error and stop the execution if we something like this?

This would be technically a breaking change but I think we are fine with it - this would need to be documented properly in the PSA documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if it is preset in both, then the tests would work as expected as it is appending both missing_recommended_fields and cim_fields at the end, so nothing would change from execution perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

So actually functionally it doesn't change anything, beside suggestion that there are fields defined as both missing_recommended_fields and cim_fields right?
If so, then if we aim for "clean" samples, then I'd break tests execution, as I think nobody will really pay an attention to warning somewhere in the middle of PSA tests execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So actually functionally it doesn't change anything, beside suggestion that there are fields defined as both missing_recommended_fields and cim_fields right?

Yes

If so, then if we aim for "clean" samples, then I'd break tests execution, as I think nobody will really pay an attention to warning somewhere in the middle of PSA tests execution.

Breaking test execution for something like this feels a bit unnecessary, but I do agree with part that warning would be hard to find between so many logs of PSA.

fields = (
list(event.requirement_test_data["cim_fields"].keys())
+ event.requirement_test_data["missing_recommended_fields"]
Expand Down
1 change: 1 addition & 0 deletions tests/e2e/addons/TA_req_broken/samples/sample_modinput.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
</cim_fields>
<missing_recommended_fields>
<field>src_user</field>
<field>dest</field>
</missing_recommended_fields>
</cim>
</event>
Expand Down
3 changes: 3 additions & 0 deletions tests/e2e/test_splunk_addon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,9 @@ def empty_method():
skipped=len(constants.TA_REQ_BROKEN_SKIPPED),
)

# checking warning for same field provided in cim_field and missing_recommended_fields
assert result.parseoutcomes().get("warnings") == 2

# make sure that we get a non '0' exit code for the testsuite as it contains failure
assert result.ret != 0

Expand Down
Loading