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

FR: Revert pytest.warns(None) changes #9402

Closed
bashtage opened this issue Dec 9, 2021 · 5 comments
Closed

FR: Revert pytest.warns(None) changes #9402

bashtage opened this issue Dec 9, 2021 · 5 comments

Comments

@bashtage
Copy link

bashtage commented Dec 9, 2021

What's the problem this feature will solve?

I need to dynamically catch warnings where whether the warning is issued or not depends on information that is only available at runtime.

Describe the solution you'd like

Accept pytest.warns(None) as equivalent to pytest.warns() without error

warn_typ = FutureWarning if dt_s_tup[0] == "nc" else None

model = VAR(endog, exog)
with pytest.warns(warn_typ):
    results_per_deterministic_terms[dt_s_tup] = model.fit(
        maxlags=4, trend=dt_s_tup[0], method="ols"
    )

from

https://github.com/statsmodels/statsmodels/blob/948bfc4520791b2d867829259367f446bd107218/statsmodels/tsa/vector_ar/tests/test_var_jmulti.py#L133-L139

Alternative Solutions

I don't see any workaround. As far as I know there is not easy way to work around this. Is the solution to use an empty *args expansion?

This removal seems to have missed the relative challenge of calling a function with 1 argument vs 0 arguments.

Additional context

@bashtage
Copy link
Author

bashtage commented Dec 9, 2021

xref #8645

@The-Compiler
Copy link
Member

See #9386. I don't see a reason to revert them.

@Zac-HD
Copy link
Member

Zac-HD commented Dec 10, 2021

Accept pytest.warns(None) as equivalent to pytest.warns() without error

Unfortunately this is off the table, because it would be a backwards-incompatible change compared to both the current and pre-deprecation behaviour (when passing None effectively disabled the check entirely).

I need to dynamically catch warnings where whether the warning is issued or not depends on information that is only available at runtime.

warn_typ = FutureWarning if dt_s_tup[0] == "nc" else None

model = VAR(endog, exog)
with pytest.warns(warn_typ):
    ...

This is precisely the point of the deprecation: your test is not currently checking that no warning is emitted in the None case! Try out

import contextlib, pytest, warnings

with pytest.warns(None):  # allows any warning!
    warnings.warn("", FutureWarning)

# The following will already work if you run tests with -Werror (a generally good idea);
# or if you have -Wignore it's equivalent!

warn_typ = FutureWarning  # or None
with pytest.warns(warn_typ) if warn_typ else contextlib.nullcontext():
    warnings.warn("", FutureWarning)

@Zac-HD Zac-HD closed this as completed Dec 10, 2021
@bashtage
Copy link
Author

bashtage commented Dec 10, 2021

@Zac-HD We can't run with -Werror because of past sins and far too many warnings int he test suite, so that isn't viable. It would be a big improvement if there was an API way to pass a value that would indicate whether there is no error expected rather than no value for this case and a value in other cases. It ultimately makes for hard to write code. Similarly with the idea of pytest.does_not_raise() which can't be used at runtime in a sane way when code might process a warning under some circumstance but not others in parameterized tests.

Unfortunately this is off the table, because it would be a backwards-incompatible change compared to both the current and pre-deprecation behaviour (when passing None effectively disabled the check entirely).

As far as I can see the pending change is also backward incompatible since it will raise an error for pytest.raises(None), so I don't see any fundamental reason why the behavior of None couldn't be made to be no warning, as many thing it is, and pytest.raises(Warning) could be made to raise any warning.

@nicoddemus
Copy link
Member

Please see #9404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants