ci, plugins: Python 3.10 and other improvements #2342
Conversation
This step is fairly easy: * add `"3.10"` in `ci.yml` (the quotes are important because YAML) * upgrade pytest to 7.1 * add an option to pytest The annoying problem with pytest 7.x is the nose plugin: activated by default, it is used as a compatiblity plugin with old codebase that used the nosetests testing framework and switched to pytest. However, for some reason, it automatically consider the `setup` (and `teardown`) function as a setup hook for test, making it into a pytest fixture, for which the first argument is the module itself. The solution is to disable the nose plugin in our configuration.
Before, we used a `setup.py` file and the "old style" Python packaging with setuptools. And we didn't used Python 3.10 yet. Now, we upgraded our build system to use the modern Python packaging (with pyproject.toml), using the new editable install, and for some reason with Python 3.10 that prevents our use of the (deprecated) `imp` builtin module. Since `imp` must be replaced by `importlib` anyway, and since we require Python 3.7 minimum, it can be safely done. The code is even simpler than before, so it's a win eitherway.
Because sopel exposes a pytest plugin that we used with an editable install, it looks like there is a conflict in Pytest: it tries to rewrite the assert from the pytest_plugin.py file, even tho it's already loaded. As a result, it generates a warning: PytestAssertRewriteWarning. This warning only means that the asserts in the file are not going to be modified, which is fine by us, as this doesn't affect third-party plugins that would use our `plugin.example` decorator. However, the warning is annoying, so we suppress it through our test configuration.
|
I've squashed and rewrote my commits into something that contains more explanation (see each commit). Since this PR adds an important milestone, I'll rebase my other draft PRs on that one. |
When running `make test` to try out some stuff on top of the py3.10 upgrade branch, I found that pytest complained about modules' __file__ attrs not matching the actual path. It seemed to be looking under the 'build' directory created upon installing the package, even in editable mode, and even though e.g. .gitignore already ignores that folder. Simplest solution was to tell pytest, "Don't do that."
Since what we're interested in is *information about* `sopel.modules` (and not actually `import_module()`-ing it), I thought I'd try using `importlib.util.find_spec()` instead of `importlib.import_module()`. And what do you know? None of the tests break. Running the bot "for real" shows it loads all the same, expected, built-in plugins. I still don't really like using `[0]`, but wanted to start small and just prove that `find_spec()` works before venturing into the "let's change stuff" arena.
Been supported since pytest 6, but we also weren't using pyproject.toml until the big packaging revamp.
|
OK, spent an hour-ish playing with this and came up with (what I think is) an improved approach, stylistically. Tests pass, and I didn't observe any difference in the plugin loading behavior as logged in DEBUG mode when starting a real bot. Along the way I also noticed a couple more improvements that could be made in this post-#2328 world. Everything is separated out into individual commits so it'll be easy to keep only some of my changes later, if there's a viable case against them. 🙂 |
I'll be the first to admit, this doesn't actually *change* the built-in plugins that are enumerated. But that was also the point: It *should* give the same list as before. Doing this is all about code hygiene. I just don't like blindly pulling the `[0]`th element of a list and ignoring the rest, even if that list only has a single entry. So the code to `find_internal_plugins()` now accounts for the possibility that there could be multiple places it has to search for those, and deduplicates the names found before trying to load them. Co-authored-by: Exirel <florian.strzelecki@gmail.com>
|
Updated with suggested change, after testing locally. Not 100% sure the behavior is identical, because I'm testing on a different environment with a different (smaller) set of plugins installed, so the numbers (loaded/failed/disabled) don't match what I checked last night. But if it breaks something, we'll know as soon as I (or @HumorBaby) update the main instance. Yeah, it would be nice if GH had a better workflow for collaborating on PRs. I'll just leave the thread open because it'll block merging by accident, for you to resolve if the force-push works for you. |
|
All good for me. 🚢 |
Description
Tin. Fix #2274.
I haven't check yet if all tests pass on older version of Python with the newest pytest, so the PR is going to do that for me here (hence, the draft).
impmodule (leftover from previous cleanup)Checklist
make qa(runsmake qualityandmake test)