fix: merge control markdown with json (#1528) #1740

d10n · 2024-10-30T14:05:33Z

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

Key links:

Sonar coverage

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.

degenaro · 2024-11-13T12:22:08Z

tests/conftest.py

@@ -259,7 +259,9 @@ def tmp_empty_cwd(tmp_path: pathlib.Path) -> Iterator[pathlib.Path]:
 @pytest.fixture(scope='function')
 def testdata_dir() -> pathlib.Path:
    """Return absolute path to test data directory."""
-    test_data_source = pathlib.Path('tests/data')
+    test_dir = pathlib.Path(__file__).parent.resolve()


Why do we switch to using __file__ ?

Expanding on the commit message for the line: to enable running / debugging unit tests in PyCharm; PyCharm sets working directories for the test runner incorrectly, even when manually specified. This is a PyCharm problem, but enabling tools is good, so why not? 😃 It also enables running unit tests from any directory in the shell.

degenaro · 2024-11-13T12:29:08Z

tests/trestle/core/commands/validate_test.py

 ) -> None:
    """Test failure modes of validation."""
+    test_data_dir = testdata_dir


Why not use test_data_dir as the parameter name and remove this statement? Or use testdata_dir in the code?

I agree this is messy. I did this to keep the changed line count small, and I was planning to do more test clean-up in a dedicated PR, but I can fix this now.

edit: I realized I didn't finish answering: the test function parameter name is associated with the fixture function name in conftest.py. The test function now uses the fixture, but the test code used this slightly different variable name for the same purpose.

degenaro · 2024-11-13T12:29:35Z

tests/trestle/core/commands/validate_test.py

    """Test successful validation runs."""
+    test_data_dir = testdata_dir


Why not use test_data_dir as the parameter name and remove this statement? Or use testdata_dir in the code?

degenaro

Is there a test case that demonstrates the failure before this PR that now passes?

d10n · 2024-11-13T12:55:41Z

Yes, looks like I dropped it in my previous rebasing. I'll include it along with the requested test_data_dir update

degenaro · 2024-11-14T16:04:18Z

@d10n In Python, __file__ is a special built-in variable that represents the path to the current script file. I still struggle with why we want to change?

Signed-off-by: d10n <[email protected]>

Function cleanup / unit tests to follow Signed-off-by: d10n <[email protected]>

Signed-off-by: d10n <[email protected]>

d10n · 2024-11-15T13:36:43Z

We discussed the unit test updates in Slack, and I am making a separate PR with them.

degenaro

LGTM.

Are PR & issue linked?

jpower432 · 2024-11-15T14:41:12Z

LGTM.

Are PR & issue linked?

@degenaro Yes. PR and issue are linked and this issue is in the current milestone. I plan to do some local testing on this and review.

jpower432

LGTM

vikas-agarwal76

LGTM

d10n force-pushed the merge-controls-md-json branch 3 times, most recently from c191240 to 230c326 Compare October 30, 2024 14:28

d10n changed the title ~~fix: Merge control markdown with json (#1528)~~ fix: merge control markdown with json (#1528) Oct 30, 2024

jpower432 requested review from degenaro, jpower432 and vikas-agarwal76 November 7, 2024 15:21

d10n force-pushed the merge-controls-md-json branch 4 times, most recently from 8e60474 to 2068811 Compare November 12, 2024 13:54

jpower432 linked an issue Nov 12, 2024 that may be closed by this pull request

Rules are not updated in Component Definition and SSP markdown when control mapping changes are made #1528

Closed

degenaro reviewed Nov 13, 2024

View reviewed changes

degenaro requested changes Nov 13, 2024

View reviewed changes

d10n force-pushed the merge-controls-md-json branch from 2068811 to 25bb44a Compare November 14, 2024 13:07

d10n requested a review from degenaro November 14, 2024 13:15

d10n force-pushed the merge-controls-md-json branch from 25bb44a to 180c8e1 Compare November 14, 2024 14:20

jpower432 requested a review from AleJo2995 November 14, 2024 20:39

d10n added 4 commits November 15, 2024 08:34

bug: add test for oscal-compass#1528

260a984

Signed-off-by: d10n <[email protected]>

fix: merge control markdown with json (oscal-compass#1528)

d8ec543

Function cleanup / unit tests to follow Signed-off-by: d10n <[email protected]>

Reuse existing merge_dicts_deep

fd737fd

Signed-off-by: d10n <[email protected]>

Fix SyntaxWarning: invalid escape sequence '\['

b06850a

Signed-off-by: d10n <[email protected]>

d10n force-pushed the merge-controls-md-json branch from d2ad415 to 1e217ee Compare November 15, 2024 13:35

Fix lint errors

1949c4c

Signed-off-by: d10n <[email protected]>

d10n force-pushed the merge-controls-md-json branch from 1e217ee to 1949c4c Compare November 15, 2024 13:35

degenaro approved these changes Nov 15, 2024

View reviewed changes

jpower432 approved these changes Nov 18, 2024

View reviewed changes

Merge branch 'develop' into merge-controls-md-json

94df588

vikas-agarwal76 approved these changes Nov 19, 2024

View reviewed changes

Merge branch 'develop' into merge-controls-md-json

8600882

jpower432 requested a review from a team as a code owner November 19, 2024 21:00

jpower432 merged commit 9f1edcf into oscal-compass:develop Nov 19, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: merge control markdown with json (#1528) #1740

fix: merge control markdown with json (#1528) #1740

d10n commented Oct 30, 2024 •

edited

Loading

degenaro Nov 13, 2024 •

edited

Loading

d10n Nov 13, 2024

degenaro Nov 13, 2024 •

edited

Loading

d10n Nov 13, 2024 •

edited

Loading

degenaro Nov 13, 2024 •

edited

Loading

degenaro left a comment

d10n commented Nov 13, 2024

degenaro commented Nov 14, 2024 •

edited

Loading

d10n commented Nov 15, 2024

degenaro left a comment

jpower432 commented Nov 15, 2024

jpower432 left a comment

vikas-agarwal76 left a comment

		"""Test successful validation runs."""
		test_data_dir = testdata_dir

fix: merge control markdown with json (#1528) #1740

fix: merge control markdown with json (#1528) #1740

Conversation

d10n commented Oct 30, 2024 • edited Loading

Types of changes

Quality assurance (all should be covered).

Summary

Key links:

Before you merge

degenaro Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

d10n Nov 13, 2024

Choose a reason for hiding this comment

degenaro Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

d10n Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

degenaro Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

degenaro left a comment

Choose a reason for hiding this comment

d10n commented Nov 13, 2024

degenaro commented Nov 14, 2024 • edited Loading

d10n commented Nov 15, 2024

degenaro left a comment

Choose a reason for hiding this comment

jpower432 commented Nov 15, 2024

jpower432 left a comment

Choose a reason for hiding this comment

vikas-agarwal76 left a comment

Choose a reason for hiding this comment

d10n commented Oct 30, 2024 •

edited

Loading

degenaro Nov 13, 2024 •

edited

Loading

degenaro Nov 13, 2024 •

edited

Loading

d10n Nov 13, 2024 •

edited

Loading

degenaro Nov 13, 2024 •

edited

Loading

degenaro commented Nov 14, 2024 •

edited

Loading