-
Notifications
You must be signed in to change notification settings - Fork 72
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
Expand Recipe testing to perform arg validation #898
Conversation
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.
Some small nits, but otherwise LGTM! I'll let you adjust if you feel like it and merge.
else: | ||
self.fail('No test_params in recipe') |
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.
This essentially makes the test_params mandatory for all recipes, even those that may not have any (as unlikely a it sounds it is something that is supported). I'm not against it, but wanted to make sure that was on your radar.
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.
Having test_params
required for every recipe was indeed the intent, but the no args edge case didn't occur to me. Updated, with test cases.
dftimewolf/lib/resources.py
Outdated
def GetTestParams(self) -> Optional[list[str]]: | ||
"""Get the test params from a recipe.""" | ||
if self.contents.get('test_params', None): | ||
return str(self.contents.get('test_params', '')).split(' ') | ||
return None |
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.
We could also always return an empty list if no test_params are found (the same truthiness as None
)
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.
Related to the other comment, changed up to recognise the difference between an empty test arg list []
and the recipe missing the test_params
field (now throws exception in the latter case.)
Expanded testing of recipes to include verifying argument validation. This should detect errors in argument handling within argument validation.
This required addition of a test invocation member
"test_params"
to each recipe, which has a nice side effect of being a "how to use this recipe" line.