-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update tests to support corrected JSON Schema Generation PR #380
Update tests to support corrected JSON Schema Generation PR #380
Conversation
7003e27
to
76c6e3c
Compare
5c4317a
to
da250ae
Compare
Previously in usnistgov#360, @kylelaker reported a needed improvement to fix a regression for usnistgov/OSCAL#1773. While I had been completing testing in usnistgov#360 and properly expanding test coverage to address how we handle enumerations based on allow-other and target attributes for field and flag elements in Metaschema (both act similarly but subtly different with more scenarios for field), I observed my test data instances needed further correction in a way that cannot be address in the PR by Kyle. This merges the changes from the commit reffed by the URL below, adds my enhancement, tests, and all examples that helped me uncover missing code. usnistgov@6fb8a74 Co-authored-by: Kyle Laker <[email protected]>
da250ae
to
a876629
Compare
@JustKuzya and @wendellpiez I had to enhance some of the tests and noticed the "sort of" and "narrow" cases for field enumerations (not flags) regressed back to not working correctly. I also included test data. I can walk everyone through this but for review it would be good to do the following.
I will soon create a wiki entry. |
@wendellpiez Dave asked in today's call that we get your review before we merge this. Do we need to talk on Monday and walk through it together or you can review it without me? |
Let's talk, pretty much at your convenience. Thanks! Main question being not the PR but the corresponding patch in the new repo https://github.com/usnistgov/metaschema-xslt. |
Can we follow up after or precede the daily status meeting?
DC
From: Wendell Piez ***@***.***>
Sent: Monday, June 26, 2023 9:09 AM
To: usnistgov/metaschema ***@***.***>
Cc: Cousin, Dmitry A. (Fed) ***@***.***>; Mention ***@***.***>
Subject: Re: [usnistgov/metaschema] Update tests to support corrected JSON Schema Generation PR (PR #380)
Let's talk, pretty much at your convenience. Thanks!
-
Reply to this email directly, view it on GitHub<#380 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACS6IVXL4YSBOAYBRNJVQN3XNGCX3ANCNFSM6AAAAAAY7XRTAQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@JustKuzya we can catch up but it seems I have a meeting after standup so perhaps we can find another time? |
OK, you seem to have the busiest calendar today - suggest the time
DC
From: A.J. Stein ***@***.***>
Sent: Monday, June 26, 2023 9:24 AM
To: usnistgov/metaschema ***@***.***>
Cc: Cousin, Dmitry A. (Fed) ***@***.***>; Mention ***@***.***>
Subject: Re: [usnistgov/metaschema] Update tests to support corrected JSON Schema Generation PR (PR #380)
Can we follow up after or precede the daily status meeting? DC
@JustKuzya<https://github.com/JustKuzya> we can catch up but it seems I have a meeting after standup so perhaps we can find another time?
-
Reply to this email directly, view it on GitHub<#380 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACS6IVWQ525LARTY57RNLYTXNGEOBANCNFSM6AAAAAAY7XRTAQ>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Kyle and AJ both/especially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks to @aj-stein-nist and reviewers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes seem to work.
- I would have prefered the WIKI (or additional Readme with tests) to have more clarification on the tests purpose, workflows tested, and expected results for each test we add.
- Also, would be nice to separate Manual testing WIKI and Automated/Terminal testing WIKI/Readme guidance
Committer Notes
This PR amends existing tests and creates new ones to expand JSON Schema generation coverage given root cause analysis and review provided by community member @kylelaker in #360.
Specifically,
json-schema-gen/json-value_flag_constrained-open_metaschema.xml
and the related test injson-schema-gen.xspec
will fail if run against current develop code (before merging #360) and will pass once run against the #360 branch (against the original commit as specific in most current PR feedback; I have not included that code to make both PRs mergeable).As-is, the test will fail with current develop as the integration test will actually return this output, which is incorrect.
The correct output expected is like the target:
The additional flag tests do not include the same number of tests because, as this developer fatefully learned last week after some unfortunate trial and error,
allowed-values
for flag elements do not have an explicit@target
attribute in their syntax. Onlyfield
s can have an explicit@target
and for good reason.All Submissions:
Changes to Core Features:
Have you updated all website](https://pages.nist.gov/metaschema) and readme documentation affected by the changes you made? Changes to the website can be made in the website/content directory of your branch.