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

Add unit tests of XCCDF 1.2 elements #9617

Merged
merged 7 commits into from
Oct 20, 2022

Conversation

jan-cerny
Copy link
Collaborator

Description:

This PR adds simple unit tests for _to_xml_element() methods of Rule, Profile, Value, and Group class in ssg.build_yaml module.

The unit test fixtures will use example data created from real-world resolved rules, profile, value, and group that were taken from build executed on current master.

Rationale:

Increases testing of build scripts.

@jan-cerny jan-cerny added the Infrastructure Our content build system label Oct 4, 2022
@jan-cerny jan-cerny added this to the 0.1.65 milestone Oct 4, 2022
@github-actions
Copy link

github-actions bot commented Oct 4, 2022

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@jan-cerny
Copy link
Collaborator Author

I believe that the 2 similar blocks of code are OK because they work with different items: rules and groups.

@matejak
Copy link
Member

matejak commented Oct 11, 2022

I am not a big fan of tests that contain an overwhelming amount of low-level asserts. Those tests may be challenging to maintain, because when an assert starts to fail, what to do? Modify the test, modify test data, or look into the source code? In some cases when the situation is unclear and time pressure is present, a wrong action may be taken.
Wouldn't it be possible to use e.g. xmldiff with pretty-printed XML reference data? If a failure occurs in such case, there will be a much simpler question - there will be no low-level asserts, only the expected XML vs obtained XML, and the decision of how to handle the discrepancy would be much more transparent.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Oct 12, 2022
It can cause tracebacks when we call a method on None object.
This doesn't happen during normal usage, but it can happen during
unit tests.
This adds simple unit tests for _to_xml_element() methods of
Rule, Profile, Value, and Group class in ssg.build_yaml module.
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Oct 13, 2022
@jan-cerny
Copy link
Collaborator Author

I have resolved the conflicts and rebased the PR on the top of the latest upstream master branch.

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

Do we want this kind of path in the repo? I agree with @matejak regarding the number low level asserts.

tests/unit/ssg-module/data/accounts_tmout.yml Outdated Show resolved Hide resolved
tests/unit/ssg-module/data/ospp.profile Outdated Show resolved Hide resolved
tests/unit/ssg-module/data/selinux.yml Outdated Show resolved Hide resolved
tests/unit/ssg-module/data/var_system_crypto_policy.yml Outdated Show resolved Hide resolved
@jan-cerny
Copy link
Collaborator Author

I have removed the hard coded definition location paths.

We will use xmldiff in unit tests of to_xml_element methods because
it allows us to store expected XML outputs as separate files and
also it allows us to detect all differences from the expected
output immediately.
The conditional import will make skip tests that require xmldiff and
lxml because xmldiff isn't present on all systems where we would like to
run our tests because xmldiff can be at this moment installed only using
pip, it isn't packaged in most of Linux distributions.
@jan-cerny
Copy link
Collaborator Author

We have discussed this to @matejak and we have found out it's also possible to use xmldiff with this type of tests it allows us to store expected XML outputs as separate files and also it allows us to detect all differences from the expected output immediately. I have add conditional imports to make the tests skip that require xmldiff and lxml when they aren't available because xmldiff isn't present on all systems where we would like to run our tests because xmldiff can be at this moment installed only using pip, it isn't packaged in most of Linux distributions.

@Mab879 Mab879 self-assigned this Oct 19, 2022
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

I like this approach better. I think we should document the need for the xmldiff python package in the building CaC doc.

Can you add a new line of the files like tests/unit/ssg-module/data/accounts_tmout.xml or will that cause issues with the XML diff?

I think we can waive the code climate duplication errors.

@jan-cerny
Copy link
Collaborator Author

I have add documentation text.

Can you add a new line of the files like tests/unit/ssg-module/data/accounts_tmout.xml or will that cause issues with the XML diff?

Unfortunately, xmldiff is sensitive on whitespace before and after tags. I don't like this behavior of xmldiff, but it seems that we will either have to live with that or invent some sort of whitespace normalization that would be executed before the test data would be passed to xmldiff.

@matejak has pointed to me https://xmldiff.readthedocs.io/en/stable/api.html#using-formatters but we have found that it's related to the diff output instead of input. Then I did an experiment where I add a newline after a tag to the test data and whatever argument I put into this formatter class the diff still showed the newline. 😿

@jan-cerny
Copy link
Collaborator Author

Unfortunately, xmldiff is sensitive on whitespace before and after tags.

But now, an unexpected twist: we have found that this isn't true for the diff_files method. When using the diff_trees it is sensitive on whitespace and when using diff_files method it isn't sensitive on whitespace.

So we can save the actual outputs produced by the SUT to temporary files and then compare the temporary files with the test data files using the diff_files.

Originally, we thought xmldiff is sensitive on whitespace before and
after tags. But now we have found that this isn't true for the
diff_files method. When using the diff_trees it is sensitive on
whitespace and when using diff_files method it isn't sensitive on
whitespace.

So we can save the actual outputs produced by the SUT to temporary files
and then compare the temporary files with the test data files using the
diff_files.
@jan-cerny
Copy link
Collaborator Author

I have se diff_files instead of diff_trees

@codeclimate
Copy link

codeclimate bot commented Oct 20, 2022

Code Climate has analyzed commit 4b98c1a and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 0.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 41.9% (0.6% change).

View more on Code Climate.

@Mab879 Mab879 merged commit 0f2b5c3 into ComplianceAsCode:master Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Our content build system XCCDF12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants