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

fix: xccdf to oscal-ar #1336

Merged
merged 8 commits into from
Apr 21, 2023
Merged

fix: xccdf to oscal-ar #1336

merged 8 commits into from
Apr 21, 2023

Conversation

degenaro
Copy link
Collaborator

Types of changes

  • Hot fix (emergency fix and release)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation (change which affects the documentation site)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Release (develop -> main)

Quality assurance (all should be covered).

  • My code follows the code style of this project.
  • Documentation for my change is up to date?
  • My PR meets testing requirements.
  • All new and existing tests passed.
  • All commits are signed-off.

Summary

Both task osco-result-to-oscal-ar and xccdf-result-to-oscal-ar can co-exist. The former can be removed at next major release.

Fixes issue #1329

Key links:

Before you merge

  • Ensure it is a 'squash commit' if not a release.
  • Ensure CI is currently passing
  • Check sonar. If you are working for a fork a maintainer will reach out, if required.

Signed-off-by: degenaro <lou.degenaro@gmail.com>
Signed-off-by: degenaro <lou.degenaro@gmail.com>
Signed-off-by: degenaro <lou.degenaro@gmail.com>
Signed-off-by: degenaro <lou.degenaro@gmail.com>
Signed-off-by: degenaro <lou.degenaro@gmail.com>
Signed-off-by: degenaro <lou.degenaro@gmail.com>
Copy link
Contributor

@enikonovad enikonovad left a comment

Choose a reason for hiding this comment

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

The general comment is that test data (especially for xccdf) can be generated on the fly instead of having multiple nearly identical test files. It is always easier to maintain the test data via code rather than hardcoded files

Comment on lines +61 to +69
def test_xccdf_print_info(tmp_path):
"""Test print_info call."""
config = setup_config(cf01)
section = config['task.xccdf-result-to-oscal-ar']
section['output-dir'] = str(tmp_path)
tgt = xccdf_result_to_oscal_ar.XccdfResultToOscalAR(section)
retval = tgt.print_info()
assert retval is None

Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of test cases here test similar things, I would recommend looking into @pytest.mark.parametrize to reduce the duplicated tests

Copy link
Collaborator Author

@degenaro degenaro Apr 18, 2023

Choose a reason for hiding this comment

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

Here's the thing: this is a rework of trestle task osco-result-to-oscal-ar to change its name to xccdf-result-to-oscal-ar. As much as possible I re-used existing data and code. I could upgrade the test cases here as you suggest, but that seems like a different issue than renaming the task. Eventually osco-result-to-oscal-ar should be removed, but that can be saved for the next breaking release. If you agree, I can open another issue to improve existing task test cases, since many already have this same issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, in such case we can leave it until the next time.

Copy link
Contributor

@enikonovad enikonovad left a comment

Choose a reason for hiding this comment

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

LGTM - please create an issue to remove duplicated data/tests

@degenaro
Copy link
Collaborator Author

Issue opened #1362

@degenaro degenaro merged commit 7305883 into develop Apr 21, 2023
15 checks passed
@degenaro degenaro deleted the fix/xccdf-to-oscal-ar branch April 21, 2023 11:59
@AleJo2995 AleJo2995 mentioned this pull request May 12, 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

2 participants