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

[Good first issue] TST: Disallow bare pytest.raises #30999

Closed
7 of 21 tasks
ShaharNaveh opened this issue Jan 14, 2020 · 84 comments · Fixed by #38973
Closed
7 of 21 tasks

[Good first issue] TST: Disallow bare pytest.raises #30999

ShaharNaveh opened this issue Jan 14, 2020 · 84 comments · Fixed by #38973
Labels
Code Style Code style, linting, code_checks good first issue Testing pandas testing functions or related to the test suite

Comments

@ShaharNaveh
Copy link
Member

ShaharNaveh commented Jan 14, 2020

End users rely on error messages for their debugging purposes. Thus, it is important that we make sure that the correct error messages are surfaced depending on the error triggered.

The core idea is to convert this:

with pytest.raises(klass):
    # Some code that raise an error

To this:

with pytest.raises(klass, match=msg):
    # Some code that raise an error

You can read more about pytest.raises here.


Side note:

In case that the raised error message is an external error message (meaning that's not pandas specific), you should use the external_error_raised instead of pytest.raises.

the usage of external_error_raised is exactly like pytest.raises the only difference is that you don't pass in the match argument.

For example:

import pandas._testing as tm

def test_foo():
    with tm.external_error_raised(ValueError):
        raise ValueError("foo")

Keynotes:

  • Don't forget to link this issue in your PR, paste this
https://github.com/pandas-dev/pandas/issues/30999

in your PR.

  • Please comment what you are planning to work on, so we won't do double the work (no need to mention me, you can just declare what you are planning to work on, just remember to check if something is already taken).

  • If a file/files that should be marked as "done" (as if there is no more work to do), isn't marked as "done", please comment letting me know about that (And mentioning me by putting @MomIsBestFriend at the comment's body, so I'll know where to look).


To generate the full list yourself, you can add:

    -   id: unwanted-patterns-bare-pytest-raises
        name: Check for use of bare use of pytest raises
        language: python
        entry: python scripts/validate_unwanted_patterns.py --validation-type="bare_pytest_raises"
        types: [python]
        files: ^pandas/tests/
        exclude: ^pandas/tests/extension

to .pre-commit-config.yaml in the - repo: local section, and then run

pre-commit run unwanted-patterns-bare-pytest-raises --all-files.

The current list is:

  • pandas/tests/arrays/boolean/test_arithmetic.py
  • pandas/tests/computation/test_compat.py
  • pandas/tests/dtypes/test_inference.py
  • pandas/tests/indexes/multi/test_indexing.py
  • pandas/tests/indexes/multi/test_setops.py
  • pandas/tests/indexes/period/test_indexing.py
  • pandas/tests/indexes/test_common.py
  • pandas/tests/indexes/test_numpy_compat.py
  • pandas/tests/indexing/multiindex/test_partial.py
  • pandas/tests/indexing/test_coercion.py
  • pandas/tests/io/test_sql.py
  • pandas/tests/libs/test_hashtable.py
  • pandas/tests/reductions/test_reductions.py
  • pandas/tests/reductions/test_stat_reductions.py
  • pandas/tests/resample/test_resampler_grouper.py
  • pandas/tests/reshape/test_get_dummies.py
  • pandas/tests/reshape/test_union_categoricals.py
  • pandas/tests/series/apply/test_series_apply.py
  • pandas/tests/series/test_ufunc.py
  • pandas/tests/window/moments/test_moments_ewm.py
  • pandas/tests/window/test_apply.py

NOTE:

The list may change as files are moved/renamed constantly.


Took pretty much everything from #23922, that was originally opened by @gfyoung.

@ShaharNaveh ShaharNaveh added good first issue Code Style Code style, linting, code_checks Testing pandas testing functions or related to the test suite labels Jan 14, 2020
@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 14, 2020

I'll take:

  • pandas/tests/test_common.py
  • pandas/tests/test_downstream.py
  • pandas/tests/test_errors.py
  • pandas/tests/test_lib.py
  • pandas/tests/test_take.py
  • pandas/tests/internals/test_internals.py
  • pandas/tests/window/test_rolling.py

@gdex1
Copy link
Contributor

gdex1 commented Jan 14, 2020

I will begin working on:

pandas/tests/arithmetic/test_numeric.py
pandas/tests/arithmetic/test_object.py
pandas/tests/arithmetic/test_period.py
pandas/tests/arithmetic/test_timedelta64.py
pandas/tests/arrays/interval/test_interval.py

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 14, 2020

@gdex1 I hope this will help you :)

(The numbers represents line number)

pandas/tests/arithmetic/test_numeric.py:138
pandas/tests/arithmetic/test_numeric.py:141
pandas/tests/arithmetic/test_numeric.py:190
pandas/tests/arithmetic/test_numeric.py:208
pandas/tests/arithmetic/test_numeric.py:210
pandas/tests/arithmetic/test_numeric.py:212
pandas/tests/arithmetic/test_numeric.py:214
pandas/tests/arithmetic/test_numeric.py:232
pandas/tests/arithmetic/test_numeric.py:234
pandas/tests/arithmetic/test_numeric.py:236
pandas/tests/arithmetic/test_numeric.py:238
pandas/tests/arithmetic/test_numeric.py:519
pandas/tests/arithmetic/test_numeric.py:610
pandas/tests/arithmetic/test_numeric.py:615
pandas/tests/arithmetic/test_numeric.py:617
pandas/tests/arithmetic/test_numeric.py:795
pandas/tests/arithmetic/test_numeric.py:798
pandas/tests/arithmetic/test_numeric.py:819
pandas/tests/arithmetic/test_object.py:140
pandas/tests/arithmetic/test_object.py:152
pandas/tests/arithmetic/test_object.py:154
pandas/tests/arithmetic/test_object.py:278
pandas/tests/arithmetic/test_object.py:280
pandas/tests/arithmetic/test_object.py:282
pandas/tests/arithmetic/test_object.py:284
pandas/tests/arithmetic/test_object.py:298
pandas/tests/arithmetic/test_object.py:301
pandas/tests/arithmetic/test_object.py:315
pandas/tests/arithmetic/test_object.py:318
pandas/tests/arithmetic/test_timedelta64.py:51
pandas/tests/arithmetic/test_timedelta64.py:445
pandas/tests/arithmetic/test_timedelta64.py:607
pandas/tests/arithmetic/test_timedelta64.py:609
pandas/tests/arithmetic/test_timedelta64.py:703
pandas/tests/arithmetic/test_timedelta64.py:705
pandas/tests/arithmetic/test_timedelta64.py:707
pandas/tests/arithmetic/test_timedelta64.py:709
pandas/tests/arithmetic/test_timedelta64.py:741
pandas/tests/arithmetic/test_timedelta64.py:743
pandas/tests/arithmetic/test_timedelta64.py:960
pandas/tests/arithmetic/test_timedelta64.py:972
pandas/tests/arithmetic/test_timedelta64.py:1028
pandas/tests/arithmetic/test_timedelta64.py:1037
pandas/tests/arithmetic/test_timedelta64.py:1039
pandas/tests/arithmetic/test_timedelta64.py:1502
pandas/tests/arithmetic/test_timedelta64.py:1505
pandas/tests/arithmetic/test_timedelta64.py:1508
pandas/tests/arithmetic/test_timedelta64.py:1511
pandas/tests/arithmetic/test_timedelta64.py:1536
pandas/tests/arithmetic/test_timedelta64.py:1591
pandas/tests/arithmetic/test_timedelta64.py:1783
pandas/tests/arithmetic/test_timedelta64.py:1785
pandas/tests/arithmetic/test_timedelta64.py:1911
pandas/tests/arithmetic/test_timedelta64.py:1960
pandas/tests/arithmetic/test_timedelta64.py:1962
pandas/tests/arithmetic/test_timedelta64.py:1968
pandas/tests/arrays/interval/test_interval.py:155

@gfyoung gfyoung pinned this issue Jan 14, 2020
@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 14, 2020

@gfyoung the list wasn't generated by grep -r -e "pytest.raises([a-zA-Z]*)" pandas/tests -l in fact, it was generated by the script in #30755 (a validation type called bare_pytest_raises), I will put instructions at the issue body, once it gets merged 😄

@shubchat
Copy link
Contributor

@MomIsBestFriend I will help with :
pandas/tests/base/test_constructors.py
pandas/tests/base/test_ops.py

@DylanBrdt
Copy link

I can take care of these:
@MomIsBestFriend

pandas/tests/io/test_html.py
pandas/tests/io/test_parquet.py
pandas/tests/io/test_sql.py
pandas/tests/io/test_stata.py
pandas/tests/plotting/test_backend.py
pandas/tests/plotting/test_boxplot_method.py
pandas/tests/plotting/test_frame.py
pandas/tests/plotting/test_hist_method.py
pandas/tests/plotting/test_series.py
pandas/tests/reductions/test_reductions.py

DylanBrdt added a commit to DylanBrdt/pandas that referenced this issue Jan 15, 2020
@jorisvandenbossche
Copy link
Member

@MomIsBestFriend there was quite some discussion in #23922 about to go about this. Because to repeat as I said there: I don't think we should "blindly" assert all error messages.

Some things that were said in that thread: limit it to internal error messages, limit the match to a few key words of the message, avoid complicated patterns.

Also, I think asserting error messages should go hand in hand with actually checking if it is a good, clear error message, and potentially improving this.

It might be good to distill a list of attention points from the discussion in the other issue to put here.

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 15, 2020

@jorisvandenbossche

@MomIsBestFriend there was quite some discussion in #23922 about to go about this. Because to repeat as I said there: I don't think we should "blindly" assert all error messages.

I completely agree, but the problem is that newcomers don't know what error messages to assert and what error messages not to assert, if we somehow define rules on what error messages to assert and what not to assert, and at the same time keeping this issue "beginner friendly", it will be great (IMO).

Also, if we plan to enforce this in the CI we need to somehow mark what bare pytest raises are "bare" on purpose (IMO comment with the style of isort: skip is enough) , and also so other people will know that a particular bare pytest raise is bare on purpose.

Some things that were said in that thread: limit it to internal error messages, limit the match to a few key words of the message, avoid complicated patterns.

I don't see why we wouldn't want to test internal error messages, can you please elaborate even more?

I see the point that you pointed out in #23922 (comment), and I'm +1 on that, but I'm +2 (if that make any sense) on #23922 (comment) and #23922 (comment) because IMO the benefit is larger than the cost.

Also, I think asserting error messages should go hand in hand with actually checking if it is a good, clear error message, and potentially improving this.

Absolutely agree.

It might be good to distill a list of attention points from the discussion in the other issue to put here.

I have read the conversation at #23922, but I didn't saw anything that IMO is worth putting as a "note" in the issue's body, can you please point out things I missed?

@gfyoung
Copy link
Member

gfyoung commented Jan 15, 2020

I have read the conversation at #23922, but I didn't saw anything that IMO is worth putting as a "note" in the issue's body, can you please point out things I missed?

I don't see much else to add from that issue either.

I completely agree, but the problem is that newcomers don't know what error messages to assert and what error messages not to assert, if we somehow define rules on what error messages to assert and what not to assert, and at the same time keeping this issue "beginner friendly", it will be great (IMO).

Also, if we plan to enforce this in the CI we need to somehow mark what bare pytest raises are "bare" on purpose (IMO comment with the style of isort: skip is enough) , and also so other people will know that a particular bare pytest raise is bare on purpose.

These are reasons in part why picking and choosing which to test and which not to test is not the direction I would prefer. I would also add that we do sometimes check error message strings in except blocks, so good error messages also benefit us during development.

Also, if these "internal" messages aren't that important, why do we have an error message in the first place? I would then just create a helper that then asserts the message is empty.

@jorisvandenbossche
Copy link
Member

I don't see why we wouldn't want to test internal error messages, can you please elaborate even more?

So I said "limit to internal error messages", while "internal" can be a bit ambiguous... I meant: error messages that originate from pandas itself, and of course we want to test those. But so I meant that we (IMO) shouldn't test too much external error messages, meaning: messages coming from eg numpy or other libraries. Numpy can change those, and then our tests start failing due to a cosmetic change in numpy (and this is not hypothetical, it happened just last week I think).

Now, I used "internal" in a different context in #30998 (comment). There, I meant as an internal, developer oriented error message that should never be raised to the user. IMO, those are not important to test exactly with the error message.

I see the point that you pointed out in #23922 (comment), and I'm +1 on that, but I'm +2 (if that make any sense) on #23922 (comment) and #23922 (comment) because IMO the benefit is larger than the cost.

Let's put @simonjayhawkins's comment that you link to here:

I am working on the assumption, maybe incorrectly, that it will be beneficial to

  1. identify tests that can be parametrized
  2. identify tests that should be split
  3. better understanding of the failure mode tested
  4. indirectly add more documentation to the tests
  5. identify where error messages could be more consistent
  6. identify tests that are redundant
  7. help improve error messages
  8. identify tests that are currently passing for the wrong reason.

That are all useful things, I fully agree. But that is not simple, and if we want to get those things out of this issue, then this issue is not for beginners. Of course, beginners don't need to do all of those things at once, but I still have the feeling that those PRs adding asserts are often rather close to "blindly adding the current error message to the pytest.raises call" without going any further (the above points).

Also, if the above points is what makes this exercise useful, it are more concrete instructions about this that is useful to put at the top of this issue, I think.


To be clear, I am all for better error messages and better tests asserting we have and keep those error messages good. But we also have limited time, and each PR requires time and effort to do and to review, and the question is where effort is best spent.
IMO, it would be more useful instead to focus on "fix bare pytest raises" rather to focus on "improve error messages" (and while doing this, better test them).

@gfyoung
Copy link
Member

gfyoung commented Jan 16, 2020

Also, if the above points is what makes this exercise useful, it are more concrete instructions about this that is useful to put at the top of this issue, I think.

It might make sense to create a larger issue to track these (other issues worth including in such an issue are #19159 and #21575).

This part in itself is self-contained and is very approachable for beginners.

@jorisvandenbossche
Copy link
Member

@gfyoung how are those issues you link related to this discussion?

@gfyoung
Copy link
Member

gfyoung commented Jan 16, 2020

They relate to the comment that you explicitly introduced from @simonjayhawkins

@bashtage
Copy link
Contributor

#31072 contains the one missing match in stata.py

@ShaharNaveh
Copy link
Member Author

As saying here #31091 (comment) I'm with @jorisvandenbossche's idea, that we won't test error messages from external packages, Any ideas on how to mark those?

@gfyoung
Copy link
Member

gfyoung commented Jan 17, 2020

If we really don't want to test certain error messages (I could go either way on external ones to be fair), I think we should just create a helper function like this:

def external_error_raised(expected_exception):
   return pytest.raises(expected_exception, match=None)

This will make it clear to our future selves that this is a non-pandas error, and the match=None serves to appease any linting check we develop for bare pytest raises.

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 17, 2020

If we really don't want to test certain error messages (I could go either way on external ones to be fair), I think we should just create a helper function like this:

def external_error_raised(expected_exception):
   return pytest.raises(expected_exception, match=None)

This will make it clear to our future selves that this is a non-pandas error, and the match=None serves to appease any linting check we develop for bare pytest raises.

+1 on that.

I really like that idea, can we make it a convention for our tests?

That if a function is testing if a function/method is raising an error, and the error is an external error, we simply put match=None in the "pytest.raises```.

can we make it a convention for our tests?

By that I mean putting a section on in the Contributing guide.

@moink
Copy link
Member

moink commented Dec 30, 2020

I will take:

pandas/tests/reshape/test_get_dummies.py
pandas/tests/reshape/test_union_categoricals.py

@moink
Copy link
Member

moink commented Dec 30, 2020

I will take:
pandas/tests/window/moments/test_moments_ewm.py
pandas/tests/window/test_apply.py

@moink
Copy link
Member

moink commented Dec 30, 2020

I will take:

pandas/tests/dtypes/test_inference.py
pandas/tests/libs/test_hashtable.py
pandas/tests/resample/test_resampler_grouper.py

@moink
Copy link
Member

moink commented Dec 30, 2020

Done:
pandas/tests/reshape/test_get_dummies.py
pandas/tests/reshape/test_union_categoricals.py
pandas/tests/dtypes/test_inference.py
pandas/tests/libs/test_hashtable.py
pandas/tests/resample/test_resampler_grouper.py

@moink
Copy link
Member

moink commented Dec 30, 2020

Done:
pandas/tests/window/moments/test_moments_ewm.py
pandas/tests/window/test_apply.py

@moink
Copy link
Member

moink commented Dec 30, 2020

I will take

pandas/tests/indexing/multiindex/test_partial.py
pandas/tests/indexing/test_coercion.py

@moink
Copy link
Member

moink commented Dec 31, 2020

I will take
pandas/tests/arrays/boolean/test_arithmetic.py

@moink
Copy link
Member

moink commented Dec 31, 2020

I will make an attempt at:

pandas/tests/series/apply/test_series_apply.py
pandas/tests/series/test_ufunc.py

and see what people think

@moink
Copy link
Member

moink commented Dec 31, 2020

Done:
pandas/tests/series/apply/test_series_apply.py
pandas/tests/series/test_ufunc.py
pandas/tests/indexing/multiindex/test_partial.py
pandas/tests/indexing/test_coercion.py
pandas/tests/indexes/multi/test_indexing.py
pandas/tests/indexes/multi/test_setops.py
pandas/tests/indexes/period/test_indexing.py
pandas/tests/indexes/test_common.py
pandas/tests/indexes/test_numpy_compat.py

@moink
Copy link
Member

moink commented Dec 31, 2020

I will take
pandas/tests/io/json/test_normalize.py

@moink
Copy link
Member

moink commented Dec 31, 2020

I think I have handled all the easy-enough-to-handle instances of bare pytest.raise. There are still 15 left. Let me explain for each of them why I am not fixing them myself right now:

  • in pandas/tests/extension there are 12 spread across 6 files. If I understand the proposed linting rule, we will exclude these from the linting. It's a pretty complex inheritance hierarchy in there, and hard to tease out which tests are affected and restructure them to be able to pass the error message around to where they are actually used.
  • in pandas/tests/io/test_sql.py there are two, one in TestXMySQL.test_execute_fail and one in TestXMySQL.test_execute_closed_connection. I can't seem to figure out how to set up my local environment to get these tests to run rather than being skipped. Advice is welcome.
  • in pandas/tests/computation/test_compat.py there is 1, in test_invalid_numexpr_version. But the line in question only runs with a version of the numexpr library prior to 2.6.8 and I don't want to mess with my environment, because the last time I tried something similar I couldn't easily fix my environment and had to delete and recreate it. Again I would take advice on that.

I would say that this is no longer a "good first issue" because there are only complex instances left.

@simonjayhawkins
Copy link
Member

@moink you could submit a draft PR and use match="^$" or match="kjkkakkdkskdkd" and get the current message from the ci logs

@moink
Copy link
Member

moink commented Dec 31, 2020

@moink you could submit a draft PR and use match="^$" or match="kjkkakkdkskdkd" and get the current message from the ci logs

Ok thanks @simonjayhawkins I will do that. I thought about abusing the CI for this - if it's ok to do that then I will. I also didn't know that CI ran on draft PRs.

@moink
Copy link
Member

moink commented Jan 1, 2021

I opened #38876 with the pytest.raises context manager commented out for three tests (in pandas/tests/io/test_sql.py and pandas/tests/computation/test_compat.py) and there weren't any errors in the CI. Is it possible that the conditions for running that code is also not satisfied in the CI? In which case what's the strategy - simply delete that part of the tests?

@rhshadrach
Copy link
Member

@moink This should probably be handled on a case-by-case basis. I'd recommend searching for issues on these tests and creating them if none exist, then post references here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks good first issue Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging a pull request may close this issue.