Fix deprecation warnings

[ff137]

Such as:

• DeprecationWarning: pkg_resources is deprecated as an API
  • ♻️ Refactored to use importlib
• RemovedInMarshmallow4Warning: Passing field metadata as keyword arguments is deprecated. Use the explicit metadata=... argument instead.
  • 🎨 description and example fields must be wrapped as metadata
• various DeprecationWarnings
  • fixed by ⬆️ Upgrading jsonpath-ng, Markdown, and rlp

Large batch of pytest related warnings fixed in #2755 and #2764

The following warning is now ignored globally, as set in pyproject toml:

[tool.pytest.ini_options]
filterwarnings = [
    'ignore:distutils Version classes are deprecated. Use packaging.version instead.:DeprecationWarning', # Ignore specific DeprecationWarning for old packages using distutils version class
]

This is because quite a few external dependencies still use the old way of reading versions.

Also, test_admin_server was the primary source of other warnings, thanks to packages: apispec, aiohttp_apispec, and aiohttp_cors. We're on older versions for those packages (only the _cors one seems to not be maintained anymore), and they either raise warnings related to distutils or Marshmallow, or they complain about "NotAppKeyWarning: It is recommended to use web.AppKey instances for keys.". I tried to solve this latter warning, but couldn't quite figure it out. I think it's so minor it's fine to just ignore.

So, you'll notice I added warning ignore conditions to test_admin_server as well. And ... that seems to solve every single warning!

PR test output:

 4836 passed, 290 skipped, 6 xfailed in 205.33s (0:03:25)

No more warnings! 🎉

[ff137]

Some people just want to watch the world burn, and they merge 2500 deprecation warnings into main. 🔥
Some people have too much time on their hands, and they clean up after them. ☯️

[ff137]

@dbluhm I've added the following to the pyproject.toml:

[tool.pytest.ini_options]
filterwarnings = [
    'ignore:distutils Version classes are deprecated. Use packaging.version instead.:DeprecationWarning', # Ignore specific DeprecationWarning for old packages using distutils version class
]

This specifically ignores warning related to the old way of reading versions, used in some old external packages.

Also, test_admin_server was the primary source of other warnings, thanks to packages: apispec, aiohttp_apispec, and aiohttp_cors. We're on older versions for those packages (only the _cors one seems to not be maintained anymore), and they either raise warnings related to distutils or Marshmallow, or they complain about "NotAppKeyWarning: It is recommended to use web.AppKey instances for keys.". I tried to solve this latter warning, but couldn't quite figure it out. I think it's so minor it's fine to just ignore.

So, you'll notice I added warning ignore conditions to test_admin_server as well. And ... that seems to solve every single warning!

PR test output:

 4836 passed, 290 skipped, 6 xfailed in 205.33s (0:03:25)

No warnings!

It's now optional to raise errors for any extra warnings that come up -- in which case they will have to be solved, or explicitly ignored as well. If we opt for this, to raise error on warnings, then I can remove that workflow edit which explicitly errors on coroutine not awaited. I think that's preferable, but all contributors will have to be on the same page, that warnings must now be solved or ignored. Let me know what you think!

[dbluhm]

No warnings!

It's now optional to raise errors for any extra warnings that come up -- in which case they will have to be solved, or explicitly ignored as well. If we opt for this, to raise error on warnings, then I can remove that workflow edit which explicitly errors on coroutine not awaited. I think that's preferable, but all contributors will have to be on the same page, that warnings must now be solved or ignored. Let me know what you think!

Nice! I agree, failing on new warnings seems preferable to checks for specific warnings. We will have the maintainers meeting tomorrow. I suspect there will be little controversy given that these sorts of warnings can hide test failures but I will bring this up on that call to get everyone on the same page.

cc @swcurran

[ff137]

@dbluhm awesome - I'll add the "error on warning" option in the meantime. Can review after the aca-pug meeting 👍

[ff137]

Note: when I test locally, there's some warnings that raise errors, even though the warning was not logged previously:

Running with pytest 7.4, it's a pluggy.PluggyTeardownRaisedWarning: A plugin raised an exception during an old-style hookwrapper teardown.
With pytest 8, it's a pytest.PytestUnraisableExceptionWarning.

There's about 30 such errors... Very strange.

When I run without the "error on warning" config, then it doesn't print any warnings. So it seems there are some test teardown issues which are being absorbed, and they manifest when the error config is active.

I'll see what I can figure out, but may be some extra steps required to properly add the "error on warning" config.

Edit:

Pluggy 1.4.0 (just released) now warns about old-style hook wrappers ... This breaks any CI that treats warnings as errors.

matplotlib/pytest-mpl#216 (comment)_

[ff137]

Not a pretty solution, but I can just ignore the PluggyTeardownRaisedWarning and PytestUnraisableExceptionWarning.

Instead, I'll see if I can figure out what's wrong with the teardown logic, e.g.:

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/unittest/mock.py", line 2058, in _mock_set_magics
    setattr(_type, entry, MagicProxy(entry, self))
ResourceWarning: unclosed <socket.socket fd=12, family=AddressFamily.AF_UNIX, type=SocketKind.SOCK_STREAM, proto=0>
FAILED aries_cloudagent/messaging/jsonld/tests/test_routes.py::TestJSONLDRoutes::test_register - pytest.PytestUnraisableExceptionWarning: Exception ignored in: <socket.socket fd=-1, family=AddressFamily.AF_UNIX, type=SocketKind.SOCK_STREAM, proto=0>

[dbluhm]

@ff137 Are any of the warnings we see here solved by the upgrades in #2773?

[ff137]

@dbluhm nope, I tried both pytest 7 and 8, same behaviour of no warnings until the error config is added.

Some of the errors are non-deterministic, due to resources not being closed properly across tests. E.g. running all tests will yield ~30 errors; running those tests individually raise no errors; and then running specific modules may vary which exact test is raising the error.

So it's not so straightforward... the logs don't help isolate which resource is causing the problem.

Best I could figure out is that AioHTTPTestCase may be preferable to use in some modules, because it defines some built in teardown logic for testing aiohttp web.Applications. But I couldn't make headway, plugging away at it myself.

I think it's reasonable to put the error-on-warning config on the back burner for now, because it will require some more investigation and broader test refactoring. Also because to add the ignore condition, to solve these teardown issues, is a very generic ignore condition, which may obfuscate other legitimate warnings. So I think this is good enough for now just to fix all warnings, and then future PR test logs can more easily be reviewed to catch any new warnings that may be introduced

[swcurran]

@jamshale — in @dbluhm ’s absence, can you take a look and approve if appropriate? I think it is ready. I’ll not update the branch until the others in line are merged, so we don’t get too many unneeded integrations test runs happening.

[jamshale]

@jamshale — in @dbluhm ’s absence, can you take a look and approve if appropriate? I think it is ready. I’ll not update the branch until the others in line are merged, so we don’t get too many unneeded integrations test runs happening.

Yes. I'll go over it. I'd like to get this completed. It will keep having merge conflicts.

[jamshale]

LGTM 👍

[swcurran]

Argh... conflicts to be resolved. Sorry about that.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[swcurran]

LGTM says @jamshale — good enough for me :-)

[ff137]

Main changes are replacing pkg_resources with importlib, in config/logging.py and utils/classloader.py.
Then, fixing Marshmallow warnings (wrap descriptions in metadata).
Use logger.warning instead of .warn.
Change some test scopes to fix warnings. Wrap some tests to expect deprecation warnings.
Filter some warnings from unmaintained libraries.
And upgrade some dependencies that fix warnings.

Looks like a lot! But that's it - resolves PR tests from printing ~2500 warnings, now it's 0 :-)

[swcurran]

HUGE! Thanks!