pytest 6.0.0 raises a lot of errors related to importing

[Jackenmen]

Hi,
I don't really know what happened here, but I tried to update to pytest 6.0.0 (from pytest 5.4.3) and running tests now ends up in 13MB log of errors. Only other info I can give is that using --import-mode=importlib flag fixes it.
I know this isn't incredibly helpful description, but I honestly have no idea what happened there, I can answer any follow up questions if needed.

Logs of failing run without any flags: https://github.com/jack1142/Red-DiscordBot/commit/9b2437278a64a303193717e8ce1ec197e3de1f25/checks/921018915/logs

Logs of successful run with --import-mode=importlib flag: https://github.com/jack1142/Red-DiscordBot/commit/e6f033855c69854e3be8401df9c62729d60054a5/checks/921089665/logs

[sivel]

cc @relrod @nitzmahone @mattclay

[nitzmahone]

For Ansible, it was ab6dacf that broke us (thanks @relrod for tracking that down!). The hack we use for locating test modules under PEP420 packages was broken by the impl change away from py.path (since we were monkeypatching it ala https://stackoverflow.com/questions/50174130/how-do-i-pytest-a-project-using-pep-420-namespace-packages/50175552#50175552 ... yes, yes, we know ;) ). Ansible can't cut over to --import-mode=importlib at the moment, since we're using an "old-style" custom loader that doesn't implement find_spec, and we also still need to work with older pytest for awhile longer on Python 2.7. Once we've dropped 2.7 controller support, we might be able to alter our loader to implement find_spec, or maybe pytest would consider accepting a PR that modifies the new importlib stuff to fall back to the importlib bridge code for old-style loaders, and maybe a couple other tweaks to make PEP420 stuff work out of the box... Anyway, we've fixed this for now by conditionally monkeypatching the new _pytest.pathlib.resolve_package_path in ansible/ansible#70963.

BTW- thanks @nicoddemus for adding --import-mode=importlib- once we can take advantage, that should be a much cleaner solution!

[Jackenmen]

Thanks for commenting @nitzmahone, this made me look if we have any usage of _pytest ourselves and... We do! Apparently we've done this awful monkey-patching on pathlib.Path.relative_to(); technically it's been done using monkeysession fixture, but it must have "leaked" out of tests it was meant for therefore causing this issue.
We weren't actually using this monkey-patching anymore so I was able to just remove that code entirely:
5e0bd9f (#4126)

I'm leaving this open for now, but it's probably entirely on us for doing what we were doing.

[nitzmahone]

Ditto, we know we're doing a Bad Thing, but I haven't looked to see if we could get the same result with something like a custom collector. Doubtful we could do it "properly" with so little code, just at the cost of it being really brittle... 😑

[The-Compiler]

From a quick look at the logs @jack1142 posted, it seems like pytest is trying to import a module called /.home.runner.work.Red-DiscordBot.Red-DiscordBot.tests.core which (obviously) fails with ModuleNotFoundError: No module named '/'. Note it only happens after a couple of tests, and the rootdir is correct:

============================= test session starts ==============================
platform linux -- Python 3.8.3, pytest-6.0.0, py-1.9.0, pluggy-0.13.1
cachedir: .tox/py/.pytest_cache
rootdir: /home/runner/work/Red-DiscordBot/Red-DiscordBot
plugins: Red-DiscordBot-3.3.11.dev1, asyncio-0.14.0, mock-3.2.0, aiohttp-json-rpc-0.13.1
collected 206 items

tests/cogs/test_alias.py .......                                         [  3%]
tests/cogs/test_economy.py ........                                      [  7%]
tests/cogs/test_mod.py ...                                               [  8%]
tests/cogs/test_permissions.py .                                         [  9%]
tests/cogs/test_trivia.py .                                              [  9%]
tests/cogs/downloader/test_downloader.py ......................          [ 20%]
tests/cogs/downloader/test_git.py ................................       [ 35%]
tests/cogs/downloader/test_installable.py .......                        [ 39%]
tests/core/test_cog_manager.py sEEEEE                                    [ 42%]
tests/core/test_commands.py EEE                                          [ 43%]
tests/core/test_config.py EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE [ 66%]
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE                              [ 86%]
tests/core/test_data_manager.py Es                                       [ 87%]
tests/core/test_installation.py E                                        [ 88%]
tests/core/test_rpc.py EEEEEEEEEEE                                       [ 93%]
tests/core/test_utils.py EEEEEEEsE                                       [ 98%]
tests/core/test_version.py EEEE                                          [100%]

The code tested by tests/core/test_cog_manager.py (where things start falling apart) does seem to do custom importing of plugins or something, so I'm guessing something there breaks pytest.

I bisected pytest against the Red-DiscordBot testsuite and ab6dacf / #7246 ("Introduce --import-mode=importlib") broke this. cc @nicoddemus

[nicoddemus]

Thanks @The-Compiler!

Seems however that #7560 (comment) found the solution due to a monkeypatch of Path.relative_to... not sure there's anything else to do on our side and we can close this?

[Jackenmen]

I'm leaving this open for now, but it's probably entirely on us for doing what we were doing.

IMO it can be closed, but I didn't want to make that decision for the maintainers of pytest in case I missed some other issue here.

[The-Compiler]

Ah, sorry, I missed that comment. It's too hot to think today 😅

Agreed that this can be closed then, I don't think we can protect much against builtins/stdlib being patched. Also see the note at the bottom of https://docs.pytest.org/en/stable/monkeypatch.html#global-patch-example-preventing-requests-from-remote-operations.