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 tm.assert_produces_warning #58290

Open
Aloqeely opened this issue Apr 17, 2024 · 8 comments · May be fixed by #59173
Open

TST: Disallow bare tm.assert_produces_warning #58290

Aloqeely opened this issue Apr 17, 2024 · 8 comments · May be fixed by #59173
Assignees
Labels
Testing pandas testing functions or related to the test suite Warnings Warnings that appear or should be added to pandas

Comments

@Aloqeely
Copy link
Member

Aloqeely commented Apr 17, 2024

Our tests currently ensure that warnings are triggered whenever appropriate, but to ensure end users get a correct and meaningful warning message, we should also make sure the correct warning message is surfaced whenever we check for a warning.

And so I propose we start enforcing the usage of the match argument in all uses of tm.assert_produces_warning.

Using the regex assert_produces_warning\([a-zA-Z]*\), I was able to find around 200 bare uses of assert_produces_warning, and some of them are checking that warnings are not raised, so this will be an easy task to finish and would be a good way for new contributors to get familiar with the pandas codebase.

If all agree, then we can open a new issue which would have instructions for how to find the bare uses, and maybe we can enforce this using our linter. We could also implement a method tm.external_warning_produced similiar to tm.external_error_raised, which would be equivalent to using tm.assert_produces_warning(warning, match=None)

See also #23922, #30999 and #37261.

@mroeschke mroeschke added Testing pandas testing functions or related to the test suite Warnings Warnings that appear or should be added to pandas labels Apr 24, 2024
@chaarvii
Copy link
Contributor

Hey, is this still available to pick up? If yes, I'd like to take it as an open-source beginner.

@Aloqeely
Copy link
Member Author

Yes, but the easy part is mostly done.
What's left is to enforce the match argument using our linter, which might be a bit hard for a beginner and so I'd recommend working on other issues instead.

But, feel free to work on this issue if you know what you're doing. PRs will be welcomed!

@chaarvii
Copy link
Contributor

Sure, I can give it a go. As far as I can see, I need to write a new script and edit pre-commit formatter. Does this sound reasonable?

@Aloqeely
Copy link
Member Author

I've never worked with pre-commit but yeah I think it's something along these lines.

Note that when you work on this, you should exclude the uses of tm.assert_produces_warning(None) from needing the match argument since this checks that no warning is produced.

Good luck!

@chaarvii
Copy link
Contributor

Alright, thanks! I’ll assign myself to this issue.

@chaarvii
Copy link
Contributor

Take

@chaarvii
Copy link
Contributor

chaarvii commented Jul 1, 2024

Hey, is there a list of files that should be deliberately excluded from this check? I found a few on the linked PR but I'm not sure if that's the complete list.

@Aloqeely
Copy link
Member Author

Aloqeely commented Jul 1, 2024

I don't have a full list, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite Warnings Warnings that appear or should be added to pandas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants