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.

Sign up for GitHub

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

TST: check_comprehensiveness compat for --lf and -k #37402

Merged
merged 2 commits into from
Oct 29, 2020
Merged

TST: check_comprehensiveness compat for --lf and -k #37402

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
041d828
TST: check_comprehensiveness compat for --lf and -k
jbrockmendel Oct 25, 2020
3b58cd3
Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…
jbrockmendel Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions pandas/tests/indexing/test_coercion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,21 @@ def has_test(combo):
klass in x.name and dtype in x.name and method in x.name for x in cls_funcs
)

for combo in combos:
if not has_test(combo):
raise AssertionError(f"test method is not defined: {cls.__name__}, {combo}")
opts = request.config.option
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just parameterize on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

huh?

this is checking if pytest is invoked with --lf or -k foo

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, we don't do this anywhere else, so why is this actually needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

#23930 this will produce unhelpful errors when running only a subset of tests in this file

Copy link
Contributor

Choose a reason for hiding this comment

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

i saw the issue, my question is that this is either a very specific fix, in which case it might be ok, or something that actually is generally a problem. I would suggest that we could refactor the tests to simply avoid having to do this, rather than trying to address this very specifically.

Copy link
Member Author

Choose a reason for hiding this comment

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

big-picture, i think check_comprehensiveness is a good idea and we should eventually hope to use something like it in more contexts.

even if we refactored the tests in this file (by no means a small feat), if we want to have a check for comprehensiveness it would still look something like this function

Copy link
Contributor

Choose a reason for hiding this comment

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

ok fair, just don't want this to be specific here, e.g. it can easily get lost

if opts.lf or opts.keyword:
# If we are running with "last-failed" or -k foo, we expect to only
# run a subset of tests.
yield

yield
else:

for combo in combos:
if not has_test(combo):
raise AssertionError(
f"test method is not defined: {cls.__name__}, {combo}"
)

yield


class CoercionBase:
Expand Down