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.

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

JSON content extension #130

Merged
merged 4 commits into from
Mar 2, 2023
Merged

JSON content extension #130

merged 4 commits into from
Mar 2, 2023

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Feb 7, 2023

This PR expands JSON content. It also adds:

  • JSON generation tests,
  • JSON schema for tests,
  • creates an easy tool for validating JSON output.

It depends on #129

@Honny1 Honny1 mentioned this pull request Feb 7, 2023
@Honny1 Honny1 force-pushed the expand-json-content branch 3 times, most recently from 5b6b888 to 38a6cc4 Compare February 7, 2023 18:54
@Honny1 Honny1 force-pushed the expand-json-content branch 3 times, most recently from 504fe54 to 8cfcbe6 Compare February 10, 2023 10:11
@Honny1 Honny1 marked this pull request as ready for review February 10, 2023 10:17
@Honny1 Honny1 force-pushed the expand-json-content branch 2 times, most recently from 15ad63a to 9018a6d Compare February 17, 2023 10:15
def test_json_count_of_rules():
report = get_report()
json_dict = report.as_dict_for_default_json()
assert len(json_dict["rules"]) == 714
Copy link
Member

Choose a reason for hiding this comment

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

Do you think that it's the best idea to have a test file that contains 714 rules?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's okay. This test is labelled as an integration test. So it can be excluded from the normal test run using pytest -m "not integration_test" to reduce testing time. I think this test should check more than just the number of rules, but if I get too specific, the test will break after changing the JSON schema.

Comment on lines 58 to 61
if rule_id in ids_of_selected_rules:
selected_rules[rule_id] = rule
elif rule["result"] != "notselected" and not is_not_empty(ids_of_selected_rules):
selected_rules[rule_id] = rule
Copy link
Member

Choose a reason for hiding this comment

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

both branches do the same, please consolidate

@Honny1
Copy link
Member Author

Honny1 commented Feb 22, 2023

Changes:

  • Changed condition in function remove_not_selected_rules.

Copy link
Contributor

@evgenyz evgenyz left a comment

Choose a reason for hiding this comment

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

LGTM in general, just some minor things to fix.

"scan_result",
"rules"
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

No new line at the end.

def test_rearrange_identifiers(dictionary_json, result):
rearrange_identifiers(dictionary_json)
print(dictionary_json)
print("aaaa")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove that.

@pytest.mark.parametrize(
"dictionary, result",
[
({"a": "", "b": {"c": ""}, "x": "x"}, {"x": "x"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

You're testing the same "b": {"c": ""} 5 times. It does not bring any more security but drastically reduces test readability. Make sure that your cases are as unique as possible.

rearrange_identifiers(dictionary_json)
print(dictionary_json)
print("aaaa")
print(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this too, I guess.

@evgenyz evgenyz merged commit 4906c7e into OpenSCAP:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants