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

[DO NOT MERGE] Feature/test harness #2097

Closed
wants to merge 16 commits into from

Conversation

wandmagic
Copy link
Collaborator

Committer Notes

Introduce test harness for style guide and integration testing

All Submissions:

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

(For reviewers: The wiki has guidance on code review and overall issue review for completeness.)

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you included examples of how to use your new feature(s)?
  • Have you updated the OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the OSCAL-Pages and OSCAL_Reference repositories.

@wandmagic wandmagic requested a review from a team as a code owner January 22, 2025 15:54
@wandmagic wandmagic changed the base branch from main to develop January 22, 2025 16:42
@wandmagic wandmagic force-pushed the feature/test-harness branch from 45daa28 to cf39926 Compare January 22, 2025 17:05
@wendellpiez
Copy link
Contributor

@wandmagic -- I love the idea of a 'style guide' layer.

I do have a question about the use of absolute paths //constraint and //has-cardinality | ... together (when setting the context and the target for the rule). When the rule targets //has-cardinality, does this not have the effect of targeting every rule in all constraints, once for every constraint (i.e. once for every context we get from //constraint)?

However that is a Metapath question and the real problem here is that the stack does not integrate the only tool that implements this, namely oscal-cli, or am I wrong and we now have a dependency on that, and hence a way to enforce, not just integrate a style guide?

If so, do we want to add rules about constraints at the same time as the direction is supposed to be to remove constraints altogether? (Maybe yes, but this has not been asked tmk.)

Finally, isn't a style guide something that calls for more discussion? Or has discussion occurred, but team members have not all been included or briefed? This is probably a question for @iMichaela, namely how is the direction to be set here.

@wandmagic
Copy link
Collaborator Author

Yes I leave it up to nist to flesh out the style guide, my goal here is to give coverage for constraint id's being mandatory and a base line to work from to enforce formal names or any other constraints for style.

@wendellpiez
Copy link
Contributor

wendellpiez commented Jan 22, 2025

NB: there is Schematron validation for Metaschema XML instances here:

https://github.com/usnistgov/metaschema-xslt/tree/develop/src/validate

It could be extended to provide the same logic as is presented in this PR (as Schematron rather than in the form of a Metaschema constraints set). From the OSCAL repository it is available as a submodule.

@wandmagic
Copy link
Collaborator Author

NB: there is Schematron validation for Metaschema XML instances here:

https://github.com/usnistgov/metaschema-xslt/tree/develop/src/validate

It could be extended to provide the same logic as is presented in this PR (as Schematron rather than in the form of a Metaschema constraints set). From the OSCAL repository it is available as a submodule.

I was able to completely deprecate the use of metaschama-xslt locally, so I'd rather not re-introduce it, but if you wish to leverage it instead feel free to edit the PR.

@wendellpiez
Copy link
Contributor

wendellpiez commented Jan 23, 2025

Validating a metaschema (XML document) against a Schematron does not require metaschema-xslt.

More broadly, the questions raised by this PR have less to do with the files being committed, being more about alignments between the various tools, capabilities and expectations.

For example, if metaschema-xslt is no longer needed, can it be dropped from the list of submodules?

@iMichaela iMichaela changed the title Feature/test harness [DO NOT MERGE] Feature/test harness Jan 23, 2025
@iMichaela
Copy link
Contributor

@wandmagic -- I love the idea of a 'style guide' layer.

I do have a question about the use of absolute paths //constraint and //has-cardinality | ... together (when setting the context and the target for the rule). When the rule targets //has-cardinality, does this not have the effect of targeting every rule in all constraints, once for every constraint (i.e. once for every context we get from //constraint)?

However that is a Metapath question and the real problem here is that the stack does not integrate the only tool that implements this, namely oscal-cli, or am I wrong and we now have a dependency on that, and hence a way to enforce, not just integrate a style guide?

If so, do we want to add rules about constraints at the same time as the direction is supposed to be to remove constraints altogether? (Maybe yes, but this has not been asked tmk.)

Finally, isn't a style guide something that calls for more discussion? Or has discussion occurred, but team members have not all been included or briefed? This is probably a question for @iMichaela, namely how is the direction to be set here.

@wendellpiez - Yes, a style guide does require a broader internal discussion and with the community. Mandatory IDs for constraints also requires discussion.

Also, based on a file change review, I see the use of metaschema-framework's oscal-cli being used everywhere in this test harness and I already explained our position several times.

Here is one example...

 - cli_type: enhanced
            cli_url: https://repo1.maven.org/maven2/dev/metaschema/oscal/oscal-cli-enhanced/2.4.0/oscal-cli-enhanced-2.4.0-oscal-cli.zip

Thank you @wandmagic - we will further discuss this PR internally. Prior discussion indicated that the purpose of this PR is to highlight the difference in features between the two versions of the oscal-cli(s) and not to actually accept any of the proposed changes. Was the scope altered?

@aj-stein-gsa
Copy link
Contributor

aj-stein-gsa commented Jan 23, 2025

Thank you @wandmagic - we will further discuss this PR internally. Prior discussion indicated that the purpose of this PR is to highlight the difference in features between the two versions of the oscal-cli(s) and not to actually accept any of the proposed changes. Was the scope altered?

@wandmagic, the maintainers of @metaschema-framework and I had a chat. That project is willing to host a harness for all users to pin version of different tools, models, and test data to evaluate. Sorry for the delay, you had asked about this possibility. Since a PR to implement that here is under further review and likely not to be accepted, I would recommend closing this PR and we can work with you to create a test harness repository there.

I hope that helps @iMichaela, not NIST staff do not have to worry about review and shared maintenance with the community of such a tool and dataset. Everyone, both in NIST and outside, can still feel free to use over there too. The project would welcome that.

EDIT: New repository can be found at metaschema-framework/oscal-test-harness, more to follow.

@wandmagic
Copy link
Collaborator Author

closing, i'll put this work in another repository

@wandmagic wandmagic closed this Jan 24, 2025
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.

4 participants