pytest.Instance not raising deprecation warning like intended?

[The-Compiler]

Personally I think it's would be better to just remove this, since trying with sqlalchemy returned no warning whatsoever, just no collection.
If it was removed we would have at least an attribute error to go by.

See also #9343 (comment)

To reproduce:

git clone https://github.com/sqlalchemy/sqlalchemy.git --depth 100
pip install git+https://github.com/pytest-dev/pytest.git
pytest -Werror

will run and print no tests ran in 13.57s, so the warning seems to not trigger

Originally posted by @CaselIT in #9277 (comment)

[The-Compiler]

If we can make it work, I think a deprecation warning definitely is preferable to removal. @bluetech do you have an idea why this wasn't triggered as intended? Maybe the access happened before pytest sets up its warning filters or something?

[RonnyPfannschmidt]

If we only trigger on usage we may miss on instance checks or pathological subclasses, I'll give the details a look later today

[CaselIT]

Thanks for tracking it!
It's probably best to update the initial post specifying the commit, since we are making sqlalchemy compatible with v7 so I would expect the reproduction case to break in a few days. Using the commit 29c5fba9ad89e53180f0bd2a026742321093105f (the current head) should work:

git clone https://github.com/sqlalchemy/sqlalchemy.git -b 29c5fba9ad89e53180f0bd2a026742321093105f
pip install git+https://github.com/pytest-dev/pytest.git
pytest -Werror

[CaselIT]

If we only trigger on usage we may miss on instance checks or pathological subclasses, I'll give the details a look later today

Looking at the code the warning is on import, so it should work. This may well be some python warning strangeness (they seem to work in unexpected ways sometimes)

[RonnyPfannschmidt]

I did a initial skim, we may need to revised how warnings are handled in setuptools plugin loading.

I suspect the sqlalchemy plugin gets imported without warning capture being in place

We can harden the code by triggering the warning on instance type checks as well

[RonnyPfannschmidt]

We ought to add a local testcase that tries a fake setuptools plugin and one using the -p somemod option

[bluetech]

So I assume PYTHONWARNINGS='error' does work, just pytest -Werror doesn't?

[bluetech]

(BTW, @CaselIT, you should apply the following patch if you're able -- pytest 7 warns about it (it has always been a mistake).

diff --git a/lib/sqlalchemy/testing/plugin/plugin_base.py b/lib/sqlalchemy/testing/plugin/plugin_base.py
index d79931b91..7bc88a14b 100644
--- a/lib/sqlalchemy/testing/plugin/plugin_base.py
+++ b/lib/sqlalchemy/testing/plugin/plugin_base.py
@@ -86,7 +86,7 @@ def setup_options(make_option):
     make_option(
         "--dbdriver",
         action="append",
-        type="string",
+        type=str,
         dest="dbdriver",
         help="Additional database drivers to include in tests.  "
         "These are linked to the existing database URLs by the "

)

[CaselIT]

It seems that using -s will print the warning, so it seems the be filtered by pytest machinary

[bluetech]

sqlalchemy sets pytest warnings to once:

https://github.com/sqlalchemy/sqlalchemy/blob/29c5fba9ad89e53180f0bd2a026742321093105f/lib/sqlalchemy/testing/warnings.py#L56-L63

If it's set to always or error then it shows up plenty :)

The pytest stuff doesn't work because sqlalchemy disables the warnings plugin:

https://github.com/sqlalchemy/sqlalchemy/blob/29c5fba9ad89e53180f0bd2a026742321093105f/setup.cfg#L93

So I think everything is working as intended.

[CaselIT]

Thanks for debugging.

That seems to have been added due to #2430, but I find it surprising that it will suppress all warning.
Even if I remove all filter warnings added by sqlalchemy, no warning shows up if -p no:warnings is added. I would expect that the normal python behaviour would be used in this case, not that no warning whatsoever is shown

[CaselIT]

(BTW, @CaselIT, you should apply the following patch if you're able -- pytest 7 warns about it (it has always been a mistake).

diff --git a/lib/sqlalchemy/testing/plugin/plugin_base.py b/lib/sqlalchemy/testing/plugin/plugin_base.py
index d79931b91..7bc88a14b 100644
--- a/lib/sqlalchemy/testing/plugin/plugin_base.py
+++ b/lib/sqlalchemy/testing/plugin/plugin_base.py
@@ -86,7 +86,7 @@ def setup_options(make_option):
     make_option(
         "--dbdriver",
         action="append",
-        type="string",
+        type=str,
         dest="dbdriver",
         help="Additional database drivers to include in tests.  "
         "These are linked to the existing database URLs by the "

)

thanks, this warning and another raised by python was also masked by the no:warnings.

I don't understand what -p no:warnings does. The documentation does not help https://docs.pytest.org/en/6.2.x/warnings.html#disabling-warning-capture-entirely. It mention that it just disables the plugin, not that all warnings would be ignored, suppressing the default python behaviour.

[bluetech]

It doesn't suppress all warnings, unless I'm missing something.

Even if I remove all filter warnings added by sqlalchemy, no warning shows up if -p no:warnings is added

By default, Python ignores deprecation warnings. You can set PYTHONWARNINGS=default or pytest -Wdefault for example to display them again.

[CaselIT]

It seems to be something sqlalchemy does. Not sure what yet, but I'll investigate.

Trying with an empty configuration does raise the warning when importing Instance.

Sorry for the noise

[The-Compiler]

No worries! I'll take a false alarm before a release rather than a real alarm a few minutes after the release any day 😉

Thanks to everyone helping to debug this! I suppose we can close this one then?

[bluetech]

I think it's working as intended, so closing.