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

parametrize_with_cases can get argnames as list #132

Merged
merged 4 commits into from Sep 7, 2020

Conversation

saroad2
Copy link
Contributor

@saroad2 saroad2 commented Sep 7, 2020

Fix #133

At the moment, the "argnames" parameter of parametrize_with_cases can only get strings as input, but it should except list of strings as well.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2020

Codecov Report

Merging #132 into master will increase coverage by 0.04%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   90.96%   91.00%   +0.04%     
==========================================
  Files         115      116       +1     
  Lines        4536     4558      +22     
==========================================
+ Hits         4126     4148      +22     
  Misses        410      410              
Impacted Files Coverage Δ
pytest_cases/fixture_parametrize_plus.py 89.38% <92.85%> (+0.13%) ⬆️
pytest_cases/case_parametrizer_new.py 89.73% <100.00%> (ø)
...test_cases/tests/cases/doc/test_parametrize_alt.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09eae11...9edea6a. Read the comment docs.

@saroad2
Copy link
Contributor Author

saroad2 commented Sep 7, 2020

Wasn't sure where to add new tests to check the new ability. please let me know where the right place is, and I'll add the missing test.

@smarie
Copy link
Owner

smarie commented Sep 7, 2020

Indeed, thanks @saroad2 !

From the pytest.mark.parametrize docstring, and from the pytest source code in ParameterSet._parse_parametrize_args, it seems that pytest performs an explicit check of isinstance(argnames, (tuple, list)).

Can you change your code so that

  • tuples and lists are accepted
  • but no other type is accepted (raise TypeError at the end of the elif statements)

Also can you please create a test file (tests/cases/doc/test_parametrize_alt.py) containing one test with a list and one test with a tuple ?

Thanks !

Copy link
Owner

@smarie smarie left a comment

Choose a reason for hiding this comment

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

Looks good to me (I just made a small fix to the second error message).
thanks @saroad2 ! I'll make the release tomorrow

@smarie smarie merged commit d9c1411 into smarie:master Sep 7, 2020
@saroad2 saroad2 deleted the argnames_can_be_lists branch September 7, 2020 19:43
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.

parametrize_with_cases should accept a list or tuple for argnames just as pytest.mark.parametrize does
3 participants