Repository navigation
Run error: true test cases and make not() fail on multiple items - #90
Open
pradeeppeddineni wants to merge 1 commit into
Open
pradeeppeddineni wants to merge 1 commit into
pradeeppeddineni wants to merge 1 commit into
Conversation
The yaml test runner never called evaluate() for cases marked error: true, so those cases always passed. It now evaluates them and expects an exception. That surfaced two real problems: - not() returned an empty collection for an input with more than one item. The spec says this is an error (singleton evaluation), and the official testNotInvalid case expects it. not() now goes through singleton(), which raises. - The math case 'Do not convert a resource node value to Date without model' does not match how plus() works today: a string that looks like a date is converted on purpose. The case now checks that behavior instead. Fixes beda-software#78
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #78
The yaml runner in tests/conftest.py never actually called evaluate() for cases marked error: true. It raised its own exception inside pytest.raises, so those cases passed no matter what the engine did. I changed it to evaluate the expression and expect it to raise, which is how it worked before af288ce.
Once that's back, 3 of the 84 error: true cases fail on master.
Two of them are a real engine bug: 5.1_existence.yaml "multi-valued coll with not() is invalid" and fhir-r4.yaml "testNotInvalid" ((1|2).not() = false). not_fn returned [] any time the input didn't have exactly one item, so not() on 2 or more items quietly gave back empty instead of failing. The spec says singleton evaluation of a collection with more than one item is an error, the official testNotInvalid case marks it invalid, and fhirpath.js does the same (its notFn goes through misc.singleton). So not_fn now just calls misc.singleton(x, "Boolean"). empty input still returns [] and more than one item now raises.
The third is 6.6_math.yaml "Do not convert a resource node value to Date without model". Like the issue says, plus() and minus() convert a date-looking string on purpose (that came in with 641f501), so the expression returns [True] without a model. I didn't touch that behavior, I just updated the case to match what the code does and renamed it so the description isn't misleading. If you'd rather drop the string fallback to line up with fhirpath.js I'm happy to switch it to that instead.
I also added tests in tests/test_evaluators.py for not() on 2 items raising and not() on an empty or single item still working. The raising ones fail on master and pass with this change. Full suite is 1669 passed on Python 3.12, and ruff check (0.6.3, the version pinned in the Pipfile) is clean for the changed files. the only thing it still reports is an existing import order note in tests/test_real.py that I left alone.