test: opt six packages in to warnings-as-errors - #612
Conversation
Add filterwarnings = ['error'] to the root [tool.pytest.ini_options] so deprecations and resource-cleanup leaks surface in CI rather than going unnoticed. Verified locally against tests/unit for the apt, passwd, pathops, snap and systemd packages -- all still pass. The nginx_k8s, rollingops and sysctl unit suites have pre-existing failures unrelated to warnings (an ops.testing import mismatch in the first two, and an assertRaises miss in sysctl) that this change does not touch. Individual packages can extend filterwarnings in their own pyproject.toml if a warning category needs to be relaxed for that package only.
Replaces the repo-wide filterwarnings = ["error"] in the root pyproject.toml with a per-package opt-in, so a package can decline (or defer) without affecting the others. The opt-in lives in <package>/tests/conftest.py rather than a [tool.pytest.ini_options] section in the package's pyproject.toml: pytest uses exactly one config file, so a package-level section would stop the root config being read at all, silently dropping --strict-markers and the shared marker list. addinivalue_line appends to the root's filters instead, and individual tests can still relax a category with @pytest.mark.filterwarnings.
The previous commit restored it from current main rather than from this branch's base, which pulled in an unrelated S311 lint ignore.
Rolling warnings-as-errors out in batches that share a reviewer.
Rolling warnings-as-errors out in batches that share a reviewer.
…ader Adds the same conftest to the cookiecutter template, .example and .tutorial, so warnings-as-errors is the default for newly created packages rather than something each one has to remember. Also switches passwd, pathops, systemd and sysctl to the full Apache header their neighbouring test files use; apt and snap keep the short form, matching theirs.
interfaces/.example is regenerated from the same template by CI and compared against the tree, so it needs the conftest too. pathops functional tests on ubuntu-22.04 fail under warnings-as-errors: an HTTPError from a 404 over the Pebble socket is never closed, so its tempfile-backed body raises ResourceWarning whenever the GC gets to it, and 156 unrelated tests fail with PytestUnraisableExceptionWarning. That's a real leak worth fixing on its own, not something to paper over with an ignore here.
pathops locks ops 3.2.0, whose Pebble client reads an HTTPError's body on a 404 but never closes it, so the tempfile behind it raises ResourceWarning whenever the GC collects it -- landing on whatever test happens to be running. ops 3.8.0 closes the error (canonical/operator 231dac90), so bump the lock and opt pathops in after all. Verified directly: with ops 3.2.0 the error body is still open after a 404 request, with 3.8.0 it is closed.
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Opting in like this seems like a pragmatic approach, and this is a good opportunity to canonicalise how libraries should define their own pytest configuration that doesn't immediately clobber the root config.
That said, I think the end goal (eventual PR) would be to have filterwarnings = ["error"] in the root pyproject.toml and then drop the corresponding line from the template and these libraries (after all the libraries with errors have their own opt outs or fixes -- I made #632 to check which fail).
In the meantime I think the template docstring should be restructured to be more general (which flows to the example and the actual package conftest.py files).
Also the snap library now has warnings that become errors -- perhaps the simplest thing would be to drop snap/tests/conftest.py from this PR and leave it as a follow-up, though I'd be happy for the fix to be included in this PR if it's straightforward.
Restructure the conftest docstrings the template generates, following review. The module docstring now covers the file generally, and pytest_configure's explains why the opt-in lives in a conftest at all -- a package-level pytest config would silently drop the root's --strict-markers and marker list -- with the warnings-as-errors rationale and its doc links moved to comments. Applied to all nine packages as well as the template, so they stay in step. Warnings-as-errors then caught a real leak in snap. When snapd resets the connection part-way through a response body, urllib has already closed the socket, but the body's file object still holds the underlying fd, so abandoning the response leaked it until the collector got round to it. _read now closes the response either way. The test that drives urllib directly, to pin down the premise that these failures arrive untranslated, needs the same treatment, since _read isn't in the picture there. Drive-by: compare a float config value with pytest.approx (RUF069). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 3xx from snapd raises before anything reads the body, so the response never reaches _read and its file object keeps the socket's fd open until the collector gets round to it -- the same leak _read was just fixed for, by a different exit. The functional suite caught it as three unclosed sockets, one per redirect case. Also compare the float config value with math.isclose rather than pytest.approx, which ruff accepts but pyright cannot type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I think that would be a good state too. If there's something people can't fix or don't want to fix they can always add an ignore, and I think the explicitness of that is better than the alternative.
They seemed pretty straightforward, so done here. Also one ruff fix as a drive-by (comparing floats with equality), which somehow seems to have slipped into main. |
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Thanks for doing this, I'm a big fan of warnings-as-errors, this will be great for these libraries.
I'd like to have the comments/docstring additions in the snap library trimmed before merging. I also wonder if there's a reason to prefer with contextlib.closing over finally: response.close() -- happy for you to make the call on that.
Co-authored-by: James Garner <james.garner@canonical.com>
_read closed the response with contextlib.closing, which meant indenting the whole try/except a level for no gain: the try was already there, and finally closes on exactly the same paths. Use finally instead, which drops the contextlib import from the module. Also trim the comments and docstrings this branch added, following review. The redirect branch and _read each keep a single line on why the response is closed, the stub's close() needs no explanation, and the test that drives urllib directly says why in one line rather than three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Should be good to merge with these suggestions applied, thanks!
Co-authored-by: James Garner <james.garner@canonical.com>
|
Huh, now the noqa is apparently not required. Maybe my local ruff is more modern and somehow I was not using the one specified here? I'll check. |
The repo pins ruff==0.11.0, which has no RUF069 rule, so the directive trips RUF100 (unused noqa, unknown rule) and fails fast-lint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I think I must have been running my more up-to-date ruff. I've removed the noqa here, but it'll need to be added at some point in the future when the charmlibs ruff version is bumped. |
2 participants
Opts six packages in to warnings-as-errors during testing — apt, passwd, pathops, snap, sysctl, and systemd — so deprecations and resource-cleanup leaks fail CI instead of scrolling past.
Per-package rather than repo-wide, so batches can go in with the reviewers who own them, and so a package can defer without holding up the rest.
rollingops,nginx_k8s, and everything underinterfaces/are deliberately untouched here. The same conftest also goes into the cookiecutter template,.exampleand.tutorial, so warnings-as-errors is the default for newly created packages rather than something each one has to remember.The opt-in lives in
<package>/tests/conftest.pyrather than a[tool.pytest.ini_options]section in each package'spyproject.toml:pytest reads exactly one config file, so a
[tool.pytest.ini_options]section in a package would stop the rootpyproject.tomlbeing read at all — that package would silently lose--strict-markersand the sharedpebble/sudo/k8s_onlymarker list, and an unknown marker would sail through collection. I checked that against real pytest rather than assuming it.addinivalue_lineappends to the root's filters instead, so the root config still applies and a package opts out by deleting one file. Individual tests can still relax a category with@pytest.mark.filterwarnings('ignore::DeprecationWarning').Refs #519