-
Notifications
You must be signed in to change notification settings - Fork 40
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 default filters #184
Add default filters #184
Conversation
Thanks @saroad2 for this nice PR. In order to ease the review process would you mind creating a very small PR with just the migration of The test is very explicative, thanks. I'm wondering how this can be documented in the file Finally note that #147 has also suggested some evolution of the regex engine (in dedicated ticket #153). I do not think that these are contradictory, that will make one more way to perform the same thing, and developers will choose whatever style they prefer. |
Hey @smarie , As you requested, I moved the Also, I added the changes to Unfortunately, I wasn't sure where to add the new API into Let me know if you want me to change anything else! |
It looks perfect. Ok I'll make the change to api reference myself once merged. Let's merge #185 first |
# Conflicts: # pytest_cases/case_funcs_new.py # pytest_cases/case_info.py
Hey @smarie , I merged master into this PR and updated Just add the new API to the documentation and this PR can be merged :) Would you mind release a new version soon so I could use those new filters? |
|
||
|
||
@parametrize_with_cases( | ||
argnames="value", cases=".", filter=filters.has_prefix("case_t") |
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.
See above: if you had written "data_t" here you would not be able to retrieve anythin, because prefix="case_"
and is applied before filters are executed. We should change this to has_id_prefix
as suggested above.
|
||
|
||
@parametrize_with_cases( | ||
argnames="value", cases=".", filter=filters.has_suffix("e") |
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.
Same comment as for prefix: rename id suffix
pytest_cases/filters.py
Outdated
def has_prefix(prefix): | ||
return CaseFilter(lambda case: case.__name__.startswith(prefix)) |
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.
The issue here is that parametrize_with_cases
already has a prefix
argument, and this is always used. Besides, it is used before the filters are called. Finally it does not take into account overridden id
s.
So I suggest that we modify this so that it acts on the case id - that will complement nicely the prefix
argument in order to work on the case id
, not on the case function name.
def has_prefix(prefix): | |
return CaseFilter(lambda case: case.__name__.startswith(prefix)) | |
def id_has_prefix(prefix): | |
"""Select cases that have a case id prefix `prefix`. | |
Note that this is not the prefix of the whole case function name, but the case id, possibly overridden with `@case(id=)`""" | |
return CaseFilter(lambda case: CaseInfo.get_from(case).id.startswith(prefix)) |
Note that tests should be updated accordingly, and the "suffix" filter too.
pytest_cases/filters.py
Outdated
def has_suffix(suffix): | ||
return CaseFilter(lambda case: case.__name__.endswith(suffix)) |
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.
def has_suffix(suffix): | |
return CaseFilter(lambda case: case.__name__.endswith(suffix)) | |
def id_has_suffix(suffix): | |
"""Select cases that have a case id suffix `suffix`. | |
Note that this is not the suffix of the whole case function name, but the case id, possibly overridden with `@case(id=)` | |
""" | |
return CaseFilter(lambda case: CaseInfo.get_from(case).id.endswith(prefix)) |
pytest_cases/filters.py
Outdated
def match_regex(regex): | ||
return CaseFilter(lambda case: re.match(regex, case.__name__)) |
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.
def match_regex(regex): | |
return CaseFilter(lambda case: re.match(regex, case.__name__)) | |
def id_match_regex(regex): | |
"""Select cases that have a case id matching `regex`. | |
Note that this is not a match of the whole case function name, but the case id, possibly overridden with `@case(id=)` | |
""" | |
return CaseFilter(lambda case: re.match(regex, CaseInfo.get_from(case).id)) |
Thanks @saroad2 , I am deeply sorry to not have spotted this earlier but there are a few issues with the filters you propose: indeed the I am sorry that this takes longer than expected, I understand that this "back and forth" probably extremely frustrating for you... But I am busy most of my time with my employers' duties, and there is a big customer delivery these days. Unfortunately I received a very bad news today: my travis.com credits are now zero, I made a request for OSS credits but no build (no release) can be done until further notice. I was planning to migrate all my travis script to nox, so this may force me to do it earlier than expected... Then once it is nox-ified I'll be independent on the build system and therefore be able to integrate in github actions, migrate back to travis, move to circleci... more freely as needed. Of course I can always do the release manually in the meantime using mkdocs deploy and twine. |
@smarie , you shouldn't be sorry at all! we are all busy people and we have previous commitments, me too. This is an open source project, and as so everyone knows that it takes time for everything to get in motion. We are not in a hurry, so take your time! As for the fixes you suggested, I totally agree. You don't need to apologise. I'll shortly make a commit with those changes. As for Travis, in my experience, I had a lot of troubles with 3rd party CI/CD tools such as CircleCI/Travis/etc. |
Codecov Report
@@ Coverage Diff @@
## master #184 +/- ##
==========================================
+ Coverage 87.59% 87.62% +0.02%
==========================================
Files 147 150 +3
Lines 5691 5745 +54
==========================================
+ Hits 4985 5034 +49
- Misses 706 711 +5
Continue to review full report at Codecov.
|
argnames="value", cases=".", filter=filters.has_tag(B) | ||
) | ||
def test_filter_with_tag(value): | ||
print(value) |
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.
print(value) | |
pass |
argnames="value", cases=".", filter=~filters.has_tag(B) | ||
) | ||
def test_filter_without_tag(value): | ||
print(value) |
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.
print(value) | |
pass |
argnames="value", cases=".", filter=filters.has_tag(B) & filters.has_tag(C) | ||
) | ||
def test_filter_with_and_relation(value): | ||
print(value) |
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.
print(value) | |
pass |
argnames="value", cases=".", filter=filters.has_tag(B) | filters.has_tag(C) | ||
) | ||
def test_filter_with_or_relation(value): | ||
print(value) |
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.
print(value) | |
pass |
argnames="value", cases=".", filter=filters.id_has_prefix("t") | ||
) | ||
def test_filter_with_prefix(value): | ||
print(value) |
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.
print(value) | |
pass |
argnames="value", cases=".", filter=filters.id_has_suffix("m") | ||
) | ||
def test_filter_with_suffix(value): | ||
print(value) |
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.
print(value) | |
pass | |
def test_synthesis(module_results_dct): | |
assert list(modeul_results_dct) == [ | |
"test_filter_with_tag[tim]", | |
"test_filter_with_tag[toni]", | |
"test_filter_without_tag[tom]", | |
"test_filter_without_tag[dom]", | |
"test_filter_with_and_relation[toni]", | |
"test_filter_with_or_relation[tim]", | |
"test_filter_with_or_relation[toni]", | |
"test_filter_with_or_relation[dom]", | |
"test_filter_with_prefix[tom]", | |
"test_filter_with_prefix[tim]", | |
"test_filter_with_prefix[toni]", | |
"test_filter_with_suffix[tom]", | |
"test_filter_with_suffix[tim]", | |
"test_filter_with_suffix[dom]", | |
] | |
@case(tags=[A, C], id="dom") | ||
def case_four(): |
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.
At least one of the examples should have the default id extracted from the function name
@case(tags=[A, C], id="dom") | |
def case_four(): | |
@case(tags=[A, C]) | |
def case_dom(): |
Thanks for your understanding @saroad2 . Final round of review: the test should be a meta-test as all others, in other words we should use pytest-harvest to collect the list of test ids in the module and test that this list is correct. The prints can be discarded. I suggested this in the comments, as you'll see. Can you make sure that it passes ? Indeed I did not try locally, I did it directly in github web page :) |
Hey @smarie . I made the final change and all tests passed locally. The only thing I didn't manage to do is to change Other than this, everything looks great :) |
Perfect, thanks @saroad2 ! Merging now |
Fixes #131
Now, pytest-cases have a bunch of default filter functions to use in order to filter cases.
Those filters can use "and" "or" and "invert" operations in order to create various complex filters.