update regression tests and run them in CI tests - #1761
Conversation
| os.path.join("tests", "resources", "dictionaries", "meddra"), | ||
| "-r", | ||
| os.path.join("tests", "resources", "Rule-CG0027.json"), | ||
| "CORE-000237", |
There was a problem hiding this comment.
can we remove the rule from resources if we are no longer using it for the tests
There was a problem hiding this comment.
It is still being used in other tests, thats why I did not remove it.
| "resources", | ||
| "CoreIssue1345", | ||
| ), | ||
| "-r", |
There was a problem hiding this comment.
I am a bit confused on this as -r is only for published rules and this is not a CORE-ID. Is CORE silently ignoring the -r and executing and that is what is being testing?
There was a problem hiding this comment.
Its the rule file actually has this
Core:
Id: CDISC.SDTMIG.CG0019
So I think it is matching correctly the core id.
There was a problem hiding this comment.
yes but why is the -r there if it executing from the local rule file? Is it not running the rule specified with -r purposefully? Trying to understand that line specifically as it relates to the test
There was a problem hiding this comment.
I have changed it so -lr directly points to that rule file, instead of using -r for selecting the rule.
There was a problem hiding this comment.
that is not the line I am referring to, -r CDISC.SDTMIG.CG0019 is what I am referring to.
-r is only for published rule IDs and CDISC.SDTMIG.CG0019 is not a CORE ID so is having that as an argument the reason for the test? Looking at the ascertions, the rule runs so I assume it is just not using the -r command so can we remove that line
| "-s", | ||
| "sdtmig", | ||
| "-v", | ||
| "3-4", |
There was a problem hiding this comment.
this test seemed to want a version of SDTMIG that doesnt contain a rule and was rewritten to not do that --was there a reason for this?
There was a problem hiding this comment.
Yes, I forgot to update that part. The real intention was to discuss this test. CoreIssue1487 required that we skip a rule if that is not part of the selected version. Now the engine raises.
cdisc_rules_engine.exceptions.custom_exceptions.LibraryMetadataNotFoundError: No library metadata found for standard 'sdtmig' version '5.0'.
We can either update the test to confirm for this exception or do we want to restore engine's functionality to skip instead of an exception?
There was a problem hiding this comment.
if you are running for an invalid standard, i would expect an exception
| break | ||
| assert target_row, "Rule CORE-000354 not present in 'Rules Report' sheet." | ||
| assert ( | ||
| target_row[4] in target_row[4] |
There was a problem hiding this comment.
this assertion will always be true. target_row[4] will never not be in target_row[4]
There was a problem hiding this comment.
updated the test
SFJohnson24
left a comment
SFJohnson24
left a comment
There was a problem hiding this comment.
a few questions--see comments
| os.path.join("tests", "resources", "dictionaries", "meddra"), | ||
| "-r", | ||
| os.path.join("tests", "resources", "Rule-CG0027.json"), | ||
| "CORE-000237", |
There was a problem hiding this comment.
I think we should continue to use the local instance of this as COREZ-000237 is an SDTMIG rule. With the split for standards with Verisian--this will have downstream issues when we have their engine for SDTM.
There was a problem hiding this comment.
see lower comment-- I think this should be reverted to the local rule file and not -r from library
SFJohnson24
left a comment
SFJohnson24
left a comment
There was a problem hiding this comment.
2 changes:
A. there are a few rules still using library and the -r command in the regression tests, can we switch them to -lr and store these rules in resources to have the rule locally so any changes to library do not break the regression.
B. can we add comments to each test case stating what it is testing and if we are not sure what it is testing/if it is redundant, can we prune the test to optimize the runtime for the tests and remove unneeded tests. This will help in the future as functionality changes/tests get stale to understand what each test is for
3 participants
The PR updates the broken regression tests and removes the regression markers from all the tests to ensure all QARegressionTests are run during the CI pipeline.