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

TST: Disallow bare pytest.raises #23922

Closed
gfyoung opened this issue Nov 26, 2018 · 44 comments
Closed

TST: Disallow bare pytest.raises #23922

gfyoung opened this issue Nov 26, 2018 · 44 comments
Labels
Code Style Code style, linting, code_checks good first issue Testing pandas testing functions or related to the test suite

Comments

@gfyoung
Copy link
Member

gfyoung commented Nov 26, 2018

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.

I would like to propose that we disallow the following:
I think we should convert the following:

with pytest.raises(klass):
   # Your code here.

And in the spirit of tm.assert_raises_regex, we should ensure that all tests have the following:

with pytest.raises(klass, match=msg):
    # Your code here.

There are likely a few corner cases that would need to be ironed out, but I think the net benefit of converting this format would be much better for the robustness of our tests.

If we are going to focus on error message quality, then let's test them consistently! 🙂

Want to open to discussion first to see if we are on board with this. If we are, then this issue will be converted to one for doing said conversions and then (maybe) enforcing in our linter.

xref #16521
xref #23550
xref #23853

===============================================================

Running this command:

grep -r -e "pytest.raises([a-zA-Z]*)" pandas/tests -l

yields the following list:

pandas/tests/api/test_types.py
pandas/tests/arithmetic/test_datetime64.py
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/categorical/test_algos.py
pandas/tests/arrays/categorical/test_analytics.py
pandas/tests/arrays/categorical/test_api.py
pandas/tests/arrays/categorical/test_constructors.py
pandas/tests/arrays/categorical/test_indexing.py
pandas/tests/arrays/categorical/test_operators.py
pandas/tests/arrays/interval/test_ops.py
pandas/tests/arrays/sparse/test_array.py
pandas/tests/arrays/sparse/test_dtype.py
pandas/tests/arrays/sparse/test_libsparse.py
pandas/tests/arrays/test_array.py
pandas/tests/arrays/test_datetimelike.py
pandas/tests/arrays/test_integer.py
pandas/tests/computation/test_compat.py
pandas/tests/computation/test_eval.py
pandas/tests/dtypes/test_cast.py
pandas/tests/dtypes/test_dtypes.py
pandas/tests/extension/base/getitem.py
pandas/tests/extension/base/ops.py
pandas/tests/extension/base/reduce.py
pandas/tests/extension/base/setitem.py
pandas/tests/extension/decimal/test_decimal.py
pandas/tests/extension/json/test_json.py
pandas/tests/extension/test_categorical.py
pandas/tests/extension/test_integer.py
pandas/tests/frame/test_analytics.py
pandas/tests/frame/test_apply.py
pandas/tests/frame/test_arithmetic.py
pandas/tests/frame/test_axis_select_reindex.py
pandas/tests/frame/test_constructors.py
pandas/tests/frame/test_convert_to.py
pandas/tests/frame/test_dtypes.py
pandas/tests/frame/test_duplicates.py
pandas/tests/frame/test_indexing.py
pandas/tests/frame/test_missing.py
pandas/tests/frame/test_mutate_columns.py
pandas/tests/frame/test_operators.py
pandas/tests/frame/test_quantile.py
pandas/tests/frame/test_query_eval.py
pandas/tests/frame/test_reshape.py
pandas/tests/frame/test_sorting.py
pandas/tests/frame/test_timeseries.py
pandas/tests/generic/test_frame.py
pandas/tests/generic/test_generic.py
pandas/tests/generic/test_series.py
pandas/tests/groupby/test_categorical.py
pandas/tests/groupby/test_groupby.py
pandas/tests/groupby/test_timegrouper.py
pandas/tests/indexes/common.py
pandas/tests/indexes/datetimelike.py
pandas/tests/indexes/datetimes/test_arithmetic.py
pandas/tests/indexes/datetimes/test_astype.py
pandas/tests/indexes/datetimes/test_construction.py
pandas/tests/indexes/datetimes/test_date_range.py
pandas/tests/indexes/datetimes/test_indexing.py
pandas/tests/indexes/datetimes/test_timezones.py
pandas/tests/indexes/datetimes/test_tools.py
pandas/tests/indexes/interval/test_astype.py
pandas/tests/indexes/interval/test_interval.py
pandas/tests/indexes/interval/test_interval_new.py
pandas/tests/indexes/interval/test_interval_tree.py
pandas/tests/indexes/multi/test_analytics.py
pandas/tests/indexes/multi/test_compat.py
pandas/tests/indexes/multi/test_constructor.py
pandas/tests/indexes/multi/test_drop.py
pandas/tests/indexes/multi/test_duplicates.py
pandas/tests/indexes/multi/test_get_set.py
pandas/tests/indexes/multi/test_indexing.py
pandas/tests/indexes/multi/test_integrity.py
pandas/tests/indexes/multi/test_monotonic.py
pandas/tests/indexes/multi/test_partial_indexing.py
pandas/tests/indexes/multi/test_sorting.py
pandas/tests/indexes/period/test_arithmetic.py
pandas/tests/indexes/period/test_construction.py
pandas/tests/indexes/period/test_indexing.py
pandas/tests/indexes/period/test_partial_slicing.py
pandas/tests/indexes/period/test_period.py
pandas/tests/indexes/test_base.py
pandas/tests/indexes/test_category.py
pandas/tests/indexes/test_common.py
pandas/tests/indexes/test_numeric.py
pandas/tests/indexes/test_range.py
pandas/tests/indexes/timedeltas/test_arithmetic.py
pandas/tests/indexes/timedeltas/test_construction.py
pandas/tests/indexes/timedeltas/test_indexing.py
pandas/tests/indexing/interval/test_interval.py
pandas/tests/indexing/interval/test_interval_new.py
pandas/tests/indexing/multiindex/test_setitem.py
pandas/tests/indexing/multiindex/test_slice.py
pandas/tests/indexing/test_categorical.py
pandas/tests/indexing/test_indexing.py
pandas/tests/indexing/test_loc.py
pandas/tests/indexing/test_panel.py
pandas/tests/indexing/test_partial.py
pandas/tests/indexing/test_scalar.py
pandas/tests/internals/test_internals.py
pandas/tests/io/formats/test_format.py
pandas/tests/io/formats/test_style.py
pandas/tests/io/formats/test_to_latex.py
pandas/tests/io/json/test_json_table_schema.py
pandas/tests/io/json/test_normalize.py
pandas/tests/io/json/test_pandas.py
pandas/tests/io/json/test_ujson.py
pandas/tests/io/parser/test_network.py
pandas/tests/io/parser/test_python_parser_only.py
pandas/tests/io/sas/test_sas7bdat.py
pandas/tests/io/test_clipboard.py
pandas/tests/io/test_common.py
pandas/tests/io/test_excel.py
pandas/tests/io/test_feather.py
pandas/tests/io/test_gcs.py
pandas/tests/io/test_html.py
pandas/tests/io/test_parquet.py
pandas/tests/io/test_pytables.py
pandas/tests/io/test_sql.py
pandas/tests/io/test_stata.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
pandas/tests/reductions/test_stat_reductions.py
pandas/tests/resample/test_base.py
pandas/tests/resample/test_period_index.py
pandas/tests/resample/test_resample_api.py
pandas/tests/reshape/merge/test_join.py
pandas/tests/reshape/merge/test_merge.py
pandas/tests/reshape/merge/test_merge_asof.py
pandas/tests/reshape/merge/test_multi.py
pandas/tests/reshape/test_concat.py
pandas/tests/reshape/test_melt.py
pandas/tests/reshape/test_pivot.py
pandas/tests/reshape/test_reshape.py
pandas/tests/reshape/test_union_categoricals.py
pandas/tests/scalar/interval/test_interval.py
pandas/tests/scalar/period/test_asfreq.py
pandas/tests/scalar/period/test_period.py
pandas/tests/scalar/timedelta/test_arithmetic.py
pandas/tests/scalar/timedelta/test_construction.py
pandas/tests/scalar/timedelta/test_timedelta.py
pandas/tests/scalar/timestamp/test_comparisons.py
pandas/tests/scalar/timestamp/test_timestamp.py
pandas/tests/scalar/timestamp/test_timezones.py
pandas/tests/scalar/timestamp/test_unary_ops.py
pandas/tests/series/indexing/test_alter_index.py
pandas/tests/series/indexing/test_boolean.py
pandas/tests/series/indexing/test_datetime.py
pandas/tests/series/indexing/test_indexing.py
pandas/tests/series/indexing/test_numeric.py
pandas/tests/series/test_alter_axes.py
pandas/tests/series/test_analytics.py
pandas/tests/series/test_api.py
pandas/tests/series/test_apply.py
pandas/tests/series/test_arithmetic.py
pandas/tests/series/test_asof.py
pandas/tests/series/test_dtypes.py
pandas/tests/series/test_missing.py
pandas/tests/series/test_operators.py
pandas/tests/series/test_replace.py
pandas/tests/series/test_timeseries.py
pandas/tests/series/test_timezones.py
pandas/tests/sparse/frame/test_frame.py
pandas/tests/sparse/test_indexing.py
pandas/tests/test_base.py
pandas/tests/test_common.py
pandas/tests/test_errors.py
pandas/tests/test_lib.py
pandas/tests/test_nanops.py
pandas/tests/test_panel.py
pandas/tests/test_take.py
pandas/tests/test_window.py
pandas/tests/tools/test_numeric.py
pandas/tests/tseries/offsets/test_offsets.py
pandas/tests/tseries/offsets/test_ticks.py
pandas/tests/tseries/test_frequencies.py
pandas/tests/tseries/test_holiday.py
pandas/tests/tslibs/test_array_to_datetime.py
pandas/tests/tslibs/test_liboffsets.py
pandas/tests/tslibs/test_timedeltas.py
pandas/tests/tslibs/test_timezones.py

@gfyoung gfyoung added Testing pandas testing functions or related to the test suite Code Style Code style, linting, code_checks Needs Discussion Requires discussion from core team before further action labels Nov 26, 2018
@datapythonista
Copy link
Member

datapythonista commented Nov 26, 2018

Just to provide some additional info, I did couple of greps, and I see that:

  • We have 3882 calls to pytest.raises
  • 1376 already have a match (in the same line as pytest,raises), leaving around 2500 cases to change
  • 301 cases have arguments of pytest.raises in multiple lines, and that could make the linting tricky

@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2018

301 cases have arguments of pytest.raises in multiple lines, and that could make the linting tricky

That's good to know. We can probably test for the existence of a closing parenthesis (in some way shape or form) is my first thought.

@jorisvandenbossche
Copy link
Member

Personally, I am not sure enforcing it here is needed. This will be a huge amount of work, which IMO would be better spent actually improving error messages.
And which does not mean that we could pay more attention to it for new code, that we try to more consistently check the message.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2018

This will be a huge amount of work

@jorisvandenbossche : I'm pretty sure we have plenty of people who would be happy to contribute to the effort, so I'm not concerned about that. 😉

@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2018

I'm okay with putting aside enforcement, but if checking error messages is a thing we should be focusing on in the future, I think converting these bare pytest.raises would still be a good thing for us (and by us, I mean contributors) to do here.

@gfyoung gfyoung added good first issue and removed Needs Discussion Requires discussion from core team before further action labels Nov 26, 2018
@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2018

@datapythonista : I'm not sure what grep's you ran, but I ran one that finds 179 files that contain bare pytest.raises as follows:

grep -r -e "pytest.raises([a-zA-Z]*)" pandas/tests -l

pandas/tests/arithmetic/test_datetime64.py
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/categorical/test_algos.py
pandas/tests/arrays/categorical/test_analytics.py
pandas/tests/arrays/categorical/test_api.py
pandas/tests/arrays/categorical/test_constructors.py
pandas/tests/arrays/categorical/test_operators.py
pandas/tests/arrays/interval/test_ops.py
pandas/tests/arrays/sparse/test_array.py
pandas/tests/arrays/sparse/test_dtype.py
pandas/tests/arrays/sparse/test_libsparse.py
pandas/tests/arrays/test_datetimelike.py
pandas/tests/arrays/test_integer.py
pandas/tests/computation/test_compat.py
pandas/tests/computation/test_eval.py
pandas/tests/dtypes/test_cast.py
pandas/tests/dtypes/test_dtypes.py
pandas/tests/extension/base/getitem.py
pandas/tests/extension/base/ops.py
pandas/tests/extension/base/reduce.py
pandas/tests/extension/decimal/test_decimal.py
pandas/tests/extension/json/test_json.py
pandas/tests/extension/test_categorical.py
pandas/tests/extension/test_integer.py
pandas/tests/frame/test_analytics.py
pandas/tests/frame/test_apply.py
pandas/tests/frame/test_arithmetic.py
pandas/tests/frame/test_axis_select_reindex.py
pandas/tests/frame/test_constructors.py
pandas/tests/frame/test_convert_to.py
pandas/tests/frame/test_dtypes.py
pandas/tests/frame/test_duplicates.py
pandas/tests/frame/test_indexing.py
pandas/tests/frame/test_missing.py
pandas/tests/frame/test_mutate_columns.py
pandas/tests/frame/test_operators.py
pandas/tests/frame/test_query_eval.py
pandas/tests/frame/test_reshape.py
pandas/tests/frame/test_sorting.py
pandas/tests/frame/test_timeseries.py
pandas/tests/generic/test_generic.py
pandas/tests/generic/test_series.py
pandas/tests/groupby/test_groupby.py
pandas/tests/groupby/test_timegrouper.py
pandas/tests/indexes/common.py
pandas/tests/indexes/datetimelike.py
pandas/tests/indexes/datetimes/test_arithmetic.py
pandas/tests/indexes/datetimes/test_construction.py
pandas/tests/indexes/datetimes/test_date_range.py
pandas/tests/indexes/datetimes/test_indexing.py
pandas/tests/indexes/datetimes/test_timezones.py
pandas/tests/indexes/datetimes/test_tools.py
pandas/tests/indexes/interval/test_astype.py
pandas/tests/indexes/interval/test_interval.py
pandas/tests/indexes/interval/test_interval_new.py
pandas/tests/indexes/interval/test_interval_tree.py
pandas/tests/indexes/multi/test_analytics.py
pandas/tests/indexes/multi/test_compat.py
pandas/tests/indexes/multi/test_constructor.py
pandas/tests/indexes/multi/test_drop.py
pandas/tests/indexes/multi/test_duplicates.py
pandas/tests/indexes/multi/test_get_set.py
pandas/tests/indexes/multi/test_indexing.py
pandas/tests/indexes/multi/test_integrity.py
pandas/tests/indexes/multi/test_monotonic.py
pandas/tests/indexes/multi/test_partial_indexing.py
pandas/tests/indexes/multi/test_sorting.py
pandas/tests/indexes/period/test_arithmetic.py
pandas/tests/indexes/period/test_construction.py
pandas/tests/indexes/period/test_indexing.py
pandas/tests/indexes/period/test_partial_slicing.py
pandas/tests/indexes/period/test_period.py
pandas/tests/indexes/test_base.py
pandas/tests/indexes/test_category.py
pandas/tests/indexes/test_common.py
pandas/tests/indexes/test_numeric.py
pandas/tests/indexes/test_range.py
pandas/tests/indexes/timedeltas/test_arithmetic.py
pandas/tests/indexes/timedeltas/test_construction.py
pandas/tests/indexes/timedeltas/test_indexing.py
pandas/tests/indexes/timedeltas/test_timedelta_range.py
pandas/tests/indexing/interval/test_interval.py
pandas/tests/indexing/interval/test_interval_new.py
pandas/tests/indexing/test_categorical.py
pandas/tests/indexing/test_indexing.py
pandas/tests/indexing/test_loc.py
pandas/tests/indexing/test_multiindex.py
pandas/tests/indexing/test_panel.py
pandas/tests/indexing/test_scalar.py
pandas/tests/internals/test_internals.py
pandas/tests/io/formats/test_format.py
pandas/tests/io/formats/test_style.py
pandas/tests/io/formats/test_to_latex.py
pandas/tests/io/json/test_json_table_schema.py
pandas/tests/io/json/test_normalize.py
pandas/tests/io/json/test_pandas.py
pandas/tests/io/json/test_ujson.py
pandas/tests/io/parser/test_network.py
pandas/tests/io/parser/test_python_parser_only.py
pandas/tests/io/parser/test_read_fwf.py
pandas/tests/io/sas/test_sas7bdat.py
pandas/tests/io/test_clipboard.py
pandas/tests/io/test_common.py
pandas/tests/io/test_excel.py
pandas/tests/io/test_feather.py
pandas/tests/io/test_gcs.py
pandas/tests/io/test_html.py
pandas/tests/io/test_parquet.py
pandas/tests/io/test_pytables.py
pandas/tests/io/test_sql.py
pandas/tests/io/test_stata.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/resample/test_base.py
pandas/tests/resample/test_datetime_index.py
pandas/tests/resample/test_period_index.py
pandas/tests/resample/test_resample_api.py
pandas/tests/reshape/merge/test_join.py
pandas/tests/reshape/merge/test_merge.py
pandas/tests/reshape/merge/test_merge_asof.py
pandas/tests/reshape/test_concat.py
pandas/tests/reshape/test_melt.py
pandas/tests/reshape/test_pivot.py
pandas/tests/reshape/test_reshape.py
pandas/tests/reshape/test_tile.py
pandas/tests/reshape/test_union_categoricals.py
pandas/tests/scalar/interval/test_interval.py
pandas/tests/scalar/period/test_asfreq.py
pandas/tests/scalar/period/test_period.py
pandas/tests/scalar/test_nat.py
pandas/tests/scalar/timedelta/test_arithmetic.py
pandas/tests/scalar/timedelta/test_construction.py
pandas/tests/scalar/timedelta/test_timedelta.py
pandas/tests/scalar/timestamp/test_comparisons.py
pandas/tests/scalar/timestamp/test_timestamp.py
pandas/tests/scalar/timestamp/test_timezones.py
pandas/tests/scalar/timestamp/test_unary_ops.py
pandas/tests/series/indexing/test_alter_index.py
pandas/tests/series/indexing/test_datetime.py
pandas/tests/series/indexing/test_indexing.py
pandas/tests/series/test_alter_axes.py
pandas/tests/series/test_analytics.py
pandas/tests/series/test_api.py
pandas/tests/series/test_apply.py
pandas/tests/series/test_arithmetic.py
pandas/tests/series/test_asof.py
pandas/tests/series/test_dtypes.py
pandas/tests/series/test_missing.py
pandas/tests/series/test_operators.py
pandas/tests/series/test_replace.py
pandas/tests/series/test_timeseries.py
pandas/tests/series/test_timezones.py
pandas/tests/sparse/frame/test_frame.py
pandas/tests/sparse/test_indexing.py
pandas/tests/test_base.py
pandas/tests/test_common.py
pandas/tests/test_errors.py
pandas/tests/test_lib.py
pandas/tests/test_nanops.py
pandas/tests/test_panel.py
pandas/tests/test_take.py
pandas/tests/test_window.py
pandas/tests/tools/test_numeric.py
pandas/tests/tseries/offsets/test_offsets.py
pandas/tests/tseries/offsets/test_ticks.py
pandas/tests/tseries/test_frequencies.py
pandas/tests/tseries/test_holiday.py
pandas/tests/tslibs/test_array_to_datetime.py
pandas/tests/tslibs/test_liboffsets.py
pandas/tests/tslibs/test_timedeltas.py
pandas/tests/tslibs/test_timezones.py
pandas/tests/util/test_hashing.py
pandas/tests/util/test_testing.py
pandas/tests/util/test_util.py

@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2018

For anyone who is interested in contributing, working on any one of the files listed above would be great!

@WillAyd
Copy link
Member

WillAyd commented Nov 26, 2018

Personally, I am not sure enforcing it here is needed. This will be a huge amount of work, which IMO would be better spent actually improving error messages.

I agree with @jorisvandenbossche here as I'm not sure how much we stand to gain out of this. There will be a lot of instances where incremental value add is nil or questionable.

@jorisvandenbossche : I'm pretty sure we have plenty of people who would be happy to contribute to the effort, so I'm not concerned about that

We already have a decent amount of PRs concerning code style and docstrings atm and the PR queue is larger than it's even been. Is this something we feel needs to be addressed now or can we come back to it once some of the noise around docstrings settles down?

@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2018

There will be a lot of instances where incremental value add is nil or questionable.

I won't rule out corner cases, but I'm pretty certain there are more instances where there will be value added. We don't have to push enforcing, but let's at least take a look and convert these. And if they aren't giving relevant error messages, then that's a good sign to change them.

Is this something we feel needs to be addressed now or can we come back to it once some of the noise around docstrings settles down?

Hardly. Otherwise, I would have tagged this for a release version. This is just another way for people to contribute to pandas if they so choose.

@jbrockmendel
Copy link
Member

I agree with everyone. This is a worthwhile goal, not an urgent goal, and a good way for newcomers to make a real contribution while learning the code.

The linting part might be easier if we required the contextmanager usage for pytest.raises. Then we should be able to identify the close-paren by the colon and newline.

To find non-context usages: re.search("(?<!with )pytest.raises", code)
To find match-less:

pattern = r"pytest.raises\((?!.*match=).*\):\n"
re.search(pattern, code, flags=re.M)

I'm sure the latter has corner cases I'm missing.

@datapythonista
Copy link
Member

@gfyoung the numbers I shared weren't number of files, but the times that pytest.raises happen.

What I'd do is create an issue to add the match to one of the files, label it as "good first issue", see how it goes (how many cases are not obvious, or it doesn't make sense to add the match, how time consuming is to make the changes and to review...). And afterwards we can continue the discussion with more information.

Does that make sense?

@bashtage
Copy link
Contributor

bashtage commented Dec 4, 2018

I don't think your regex is correct. For example, it found

            pytest.raises(
                KeyError, lambda: written_and_read_again['index_not_written'])

in test_stata

Should probably looks for pytest.raises()

@jbrockmendel
Copy link
Member

Instead of using regex, what if we patched pytest.raises to disallow the unwanted uses? We could put the patch inside of module setup/teardown functions

@gfyoung
Copy link
Member Author

gfyoung commented Dec 18, 2018

what if we patched pytest.raises to disallow the unwanted uses

@jbrockmendel : By unwanted uses, what do you mean? And patch how exactly (not sure I follow you here 100%)?

@jbrockmendel
Copy link
Member

By unwanted uses, what do you mean? And patch how exactly (not sure I follow you here 100%)?

Something along the lines of:

_raises = pytest.raises

def raises(*args, **kwargs):
    if kwargs.get("match", None) is None:
        raise LintingComplaint
    return _raises(*args, **kwargs)

def setup_module():
    pytest.raises = raises

def teardown_module():
    pytest.raises = _raises

@jbrockmendel
Copy link
Member

I'm not necessarily saying this is a good idea, just that it avoids some of the pitfalls of grep-based linting.

@mike-cramblett
Copy link
Contributor

@gfyoung
Hi there! I'm interested in working on adding the match parameters to the pytest.raises calls in those test files as my first issue. It seems like a great way to familiarize myself with testing in pandas. Is there enough consensus here for me to start? And if so should I take the first file or a chunk of them from the top of that grep list? And thanks!

@gfyoung
Copy link
Member Author

gfyoung commented Jan 23, 2019

@mike-cramblett : That's fantastic! Let me answer your questions:

Is there enough consensus here for me to start?

Yes.

And if so should I take the first file or a chunk of them from the top of that grep list?

I would suggest doing one file first and seeing how that goes. Some are trickier than others. In addition, that list is a little dated (I need to run the grep again to update the list). That being said, you can run it locally in the directory of your pandas repository to get an updated list yourself.

@mike-cramblett
Copy link
Contributor

@gfyoung

Thanks for the info and encouragement! Just reran that grep and got 162 files. I was figuring I'd start with tests/arithmetic/test_datetime64.py since it's first on the list. But it looks like that file has 94 pytest.raises calls to fix. I'm not that daunted by doing all that, since it seems they're mostly similar TypeErrors. But I'd also be open to starting on a simpler file if you have one in mind. Thanks again!

@jbrockmendel
Copy link
Member

@gfyoung are we close to the goal line here?

@jbrockmendel
Copy link
Member

wrong button

@jbrockmendel jbrockmendel reopened this Jun 11, 2019
@simonjayhawkins
Copy link
Member

@gfyoung are we close to the goal line here?

at least 1167 still to do. quite time consuming. so not close.

@WillAyd
Copy link
Member

WillAyd commented Jun 11, 2019

Question for the team - do we see any improvements for our current approach? Half of me continues to like this change as it can catch some subtle differences, but the other half of me thinks this causes unnecessary noise with CI (#26726 being an example)

@simonjayhawkins
Copy link
Member

the CI breakages are an additional burden.

but I do think that it helps identify inconsistencies in user-facing exception messages.

@WillAyd
Copy link
Member

WillAyd commented Jun 11, 2019

So two things I've thought about:

  1. We set a pseudo-standard to only pick 2-3 words max from the error message
  2. We only enforce error messages for things raised internally

With option 1 I think we should really avoid complicated patterns like this:

msg = ("(Bool column has NA values in column [0a])|"

Where we aren't actually managing expectations around what the user sees as much as implicity doing some versioning to make CI happy

For option 2 I just think failing because of a message change from an external dependency generates a lot of busy work for something we don't control anyway. Not sure how enforceable this is

@simonjayhawkins
Copy link
Member

So two things I've thought about:

  1. We set a pseudo-standard to only pick 2-3 words max from the error message
  2. We only enforce error messages for things raised internally

makes sense. maybe explicit checks for 2. and use 1 for messages raised by other packages.

that would solve the CI issues and still help improve user-facing error messages.

@gfyoung
Copy link
Member Author

gfyoung commented Jun 11, 2019

I second @simonjayhawkins suggestion as well

@jreback
Copy link
Contributor

jreback commented Jun 12, 2019

i am ok with limiting how much we check in our error messages (option 1)
option 2 are generally good at catching changing dependencies and don’t happen too often - so wouldn’t change anything here

CI failures are a canary and so good to have - even if occasionally are a false positive

@simonjayhawkins
Copy link
Member

A potential issue i can foresee with explicitly checking error messages from 3rd party packages is where the pandas test suite is used to validate environments and distributions.

After a pandas release, the test suite could fail purely due to subtle changes in the messages raised by updated dependencies.

@jbrockmendel
Copy link
Member

@gfyoung looks like we now have a code check for this. is this closed by #27354?

@gfyoung
Copy link
Member Author

gfyoung commented Oct 16, 2019

@jbrockmendel : I think there is still a fair distance to go here.

looks like we now have a code check for this

Where in the code_checks script do we have a check for this?

Here is the updated list of files that still have bare pytest.raises :

pandas/pandas/tests/io/test_html.py
pandas/pandas/tests/io/test_parquet.py
pandas/pandas/tests/io/test_sql.py
pandas/pandas/tests/io/test_stata.py
pandas/pandas/tests/plotting/test_backend.py
pandas/pandas/tests/plotting/test_boxplot_method.py
pandas/pandas/tests/plotting/test_frame.py
pandas/pandas/tests/plotting/test_hist_method.py
pandas/pandas/tests/plotting/test_series.py
pandas/pandas/tests/reductions/test_reductions.py
pandas/pandas/tests/reductions/test_stat_reductions.py
pandas/pandas/tests/reshape/merge/test_merge_asof.py
pandas/pandas/tests/reshape/merge/test_multi.py
pandas/pandas/tests/reshape/test_reshape.py
pandas/pandas/tests/reshape/test_union_categoricals.py
pandas/pandas/tests/scalar/interval/test_interval.py
pandas/pandas/tests/scalar/period/test_asfreq.py
pandas/pandas/tests/scalar/period/test_period.py
pandas/pandas/tests/scalar/timedelta/test_arithmetic.py
pandas/pandas/tests/scalar/timedelta/test_construction.py
pandas/pandas/tests/scalar/timedelta/test_timedelta.py
pandas/pandas/tests/scalar/timestamp/test_arithmetic.py
pandas/pandas/tests/scalar/timestamp/test_comparisons.py
pandas/pandas/tests/scalar/timestamp/test_timestamp.py
pandas/pandas/tests/scalar/timestamp/test_timezones.py
pandas/pandas/tests/scalar/timestamp/test_unary_ops.py
pandas/pandas/tests/series/indexing/test_alter_index.py
pandas/pandas/tests/series/test_alter_axes.py
pandas/pandas/tests/series/test_apply.py
pandas/pandas/tests/series/test_arithmetic.py
pandas/pandas/tests/series/test_asof.py
pandas/pandas/tests/series/test_constructors.py
pandas/pandas/tests/series/test_operators.py
pandas/pandas/tests/series/test_timezones.py
pandas/pandas/tests/test_base.py
pandas/pandas/tests/test_common.py
pandas/pandas/tests/test_errors.py
pandas/pandas/tests/test_lib.py
pandas/pandas/tests/test_take.py
pandas/pandas/tests/tseries/offsets/test_offsets.py
pandas/pandas/tests/tseries/offsets/test_ticks.py
pandas/pandas/tests/window/test_dtypes.py
pandas/pandas/tests/window/test_ewm.py
pandas/pandas/tests/window/test_expanding.py
pandas/pandas/tests/window/test_moments.py
pandas/pandas/tests/window/test_rolling.py
pandas/pandas/tests/window/test_timeseries_window.py
pandas/pandas/tests/window/test_window.py

@jbrockmendel
Copy link
Member

Where in the code_checks script do we have a check for this?

code_checks.sh line 136-138

@gfyoung
Copy link
Member Author

gfyoung commented Oct 17, 2019

That’s checking for a different usage of pytest.raises, which is in the non-context form e.g.

pytest.raises(Exception, lambda f: 1 / 0)

We discourage this form and prefer:

with pytest.raises(Exception):
   1 / 0

I know because I authored the PR that added the check you referenced 😉

@simonjayhawkins
Copy link
Member

superseded by #30999

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.

9 participants