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

Add run test options #610

Merged
merged 1 commit into from
Feb 5, 2018
Merged

Add run test options #610

merged 1 commit into from
Feb 5, 2018

Conversation

benjello
Copy link
Member

Thanks for contributing to OpenFisca! Remove this line, as well as any other in the following that don't fit your contribution :)

New features

  • Introduce new options --only and '--ignore` to openfisca-run-test
    • Allows for filter tthe variables to test in YAML tests

@benjello benjello requested a review from fpagnoux January 11, 2018 21:08
@benjello benjello assigned sandcha and unassigned sandcha Jan 11, 2018
@benjello
Copy link
Member Author

Submitting this PR on the behalf of @bfabre01 which didn't have the right to push brances here and got confused on the different core branches. These are convenient options when you test for particular variables on a more complete test like the one produced by the openfisca/DGFiP comparator used for examples in this branch.

@benjello
Copy link
Member Author

@Anna-Livia @sandcha @fpagnoux : any chance you can have a look at this soon ?
It is simple and easy and very helpful for users like @bfabre01 and @ClaireLeroyIPP ;-)
Thanks !

@fpagnoux fpagnoux force-pushed the add-run-test-options branch from 028cfd5 to a5f408e Compare February 5, 2018 08:00
Copy link
Member

@fpagnoux fpagnoux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me, sorry for the delay.

@benjello two changes compared to the suggested implementation:

  • The options have been renamed to be more explicit.
  • No error message when using --only-variables for a variable that is not tested in the file

Let me know if this is ok for you.

I rebased the branch on master, but @Anna-Livia @sandcha you may want to squash the commits before merging 🙂 .

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
# Changelog

## 21.2.2

Add `--only` and `--ignore` options to `openfisca-run-test` to filter out tested output variables if needed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--only seems a little too general. -n also only tests with the given name_filter in their name. What about --only-variables and --ignore-variables ?

if output_variables is not None:
try:
if test_only is not None:
for variable in test_only:
assert variable in output_variables.keys(), "La variable {} n'est pas présente dans les outputs variables du test".format(variable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to throw an error in that case ? It seems limitating : I cannot run all the rsa tests in mes-aides for instance, as an error would be raised for files that don't test rsa.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bfabre01 @ClaireLeroyIPP :

  • should we raise an error to force the user to be sure to list the right tests to test his specific variable ?
  • should we ignore tests when the variable is not present (my favorite) ?

@benjello
Copy link
Member Author

benjello commented Feb 5, 2018

@fpagnoux : c'est bon !
@sandcha @Anna-Livia si c'est bon pour vous let's merge !

@fpagnoux fpagnoux force-pushed the add-run-test-options branch from a5f408e to 3931aea Compare February 5, 2018 15:35
@fpagnoux fpagnoux merged commit 1e6ec8d into master Feb 5, 2018
@fpagnoux fpagnoux deleted the add-run-test-options branch February 5, 2018 15:40
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.

5 participants