Create unit tests for ssg.id_translate #9624
Conversation
|
Start a new ephemeral environment with changes proposed in this pull request: Fedora Environment Oracle Linux 8 Environment |
jan-cerny
force-pushed
the
unit_test_id_translate
branch
from
4593af5
to
3c8cf1e
Compare
|
I have rebased this PR on the top of the latest upstream branch to include the fix for the duplicate CCE problems. |
| @@ -21,3 +26,172 @@ def test__split_namespace(): | |||
| ns, n = sn("{}emptynamespace") | |||
| assert not ns | |||
| assert n == "emptynamespace" | |||
|
|
|||
|
|
|||
| def test_tagname_to_abbrev(): | |||
There was a problem hiding this comment.
Wouldn't it be better to define a mapping `tagname->expected abbreviation, and just iterate over the pairs?
| @@ -0,0 +1,52 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
What about editing the OVAL, so it contains less verbose and more versatile data? For example, the ID could be rewritten to RULE_ID, and things that are not tested and not required by the build system code for smooth finish could get removed from the XML file.
| return ET.parse(draft_oval_path).getroot() | ||
|
|
||
|
|
||
| def test_idtranslator_translate_oval(idtranslator, oval_tree): |
There was a problem hiding this comment.
I see that this test takes an OVAL tree, and "translates" it, whatever it means.
I would expect this test to focus on expected differences between original and translated (or expected lack of differences). Instead, the test is mainly about making sure that the translated tree has certain properties, and it does so without referencing the original.
jan-cerny
force-pushed
the
unit_test_id_translate
branch
from
3c8cf1e
to
d266320
Compare
|
I have refactor the test using a dictionary and I have removed some data from OVAL to reduce test data input and I have changed the unit tests for |
We will remove some data from the fixture used in tests of ssg.id_translate.IDTranslator.translate() method, because these data aren't important for the behavior of the method. They only add noise to the test, we should strive to form just minimal fixtures in our units. Their purpose isn't to be a valid OVAL.
When we can define a dictionary of expected results we can iterate over them in a for loop and have only 1 assert statement in the test.
This commit adds 2 new very simple unit tests for the ssg.id_translate.IDTranslator.translate() method: - test_idtranslator_translate_oval_ids - focus on translated IDs - test_idtranslator_translate_oval_differences - focus on changes in OVAL tree caused by the translator
The test test_idtranslator_translate_oval_differences was missing some asserts - let's make it more complete.
The xmldiff library compares 2 xml trees and returns a diff in a form of a list of differences which describe specific changes such as added element or changed attribute, and we can easily create a list of expected differences and compare them. Unfortunately, it works with lxml trees, so we have to convert our xml.etree.elementree trees to lxml trees. Also, it requires adding new dependencies to our CI.
jan-cerny
force-pushed
the
unit_test_id_translate
branch
from
6f831f8
to
c2a5b08
Compare
|
@matejak I have add a test that uses the xml diff using the xmldiff library. Also, I have rebased this PR on the top of the latest upstream master branch. |
Based on discussion with the team I have removed the tests that check expected properties of the output XML tree from the translate method, because we will prefer the test based on xmldiff that allows us to compare the expected diff with the actual diff.
|
Based on discussion with the team I have removed the tests that check |
|
@matejak Are there any further changes needed? |
| f'{o}:criteria/{o}:criterion[1]'), | ||
| name='test_ref', | ||
| value='oval:ssg-test_kerberos_disable_no_keytab:tst:1') | ||
| } |
There was a problem hiding this comment.
@matejak said that it would be better if the expected items are removed from the diff and finally it would be checked if the diff is empty.
We will remove each found expected item from the diff and at the end of the test we will test whether the diff is empty IOW that it doesn't contain any unexpected items.
|
I have removed the expected items from the diff |
|
Code Climate has analyzed commit 3c3d4bb and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 41.6% (0.5% change). View more on Code Climate. |
|
Thank you for the PR with those tests, I appreciate them, and we can think of any possible improvements of their readability later - it is important to start with something solid first, and this is what the PR brings. |
Description:
Create unit tests for ssg.id_translate
Rationale:
Increases testing of build scripts.