Skip to content

fix: ensure resources are cleaned up in testing.Context#2506

Open
tonyandrewmeyer wants to merge 4 commits into
canonical:mainfrom
tonyandrewmeyer:fix-resource-warnings
Open

fix: ensure resources are cleaned up in testing.Context#2506
tonyandrewmeyer wants to merge 4 commits into
canonical:mainfrom
tonyandrewmeyer:fix-resource-warnings

Conversation

@tonyandrewmeyer
Copy link
Copy Markdown
Collaborator

A few fixes to ensure we do not leak resources (until gc time) in Scenario:

  • In runtime, move the cleanup to a finally block.
  • In the Context, instead of using TemporaryDirectory, use mktmpdir directly, and register a weakref cleanup -- but also let users explicitly clean up if they prefer (and make the context manager case do that automatically).
  • In state, replace a few opens with read_texts (we could do the open with a context manager is read but this seemed the more minimal change, and none of the YAML files should be large enough that there is any difference).
  • In a couple of Scenario's own tests, avoid leaking when using Container.pull.

After these changes, the tests can be run with -Werror and not have resource warnings cause failures.

… in scenario

Make `ops.testing` test infrastructure clean up its temporary resources
deterministically so that downstream test suites can run pytest with
`-W error` without spurious ResourceWarnings:

- Context: replace `tempfile.TemporaryDirectory` with `mkdtemp` plus a
  `weakref.finalize` cleanup, and add `close()` / context-manager
  support for eager cleanup.
- Runtime._virtual_charm_root: wrap the yield in try/finally so the
  virtual charm root (or restored metadata files) is cleaned up even
  when the charm raises.
- _CharmSpec: use `Path.read_text()` instead of leaving `Path.open()`
  file handles for the GC to close.
- test_fs_pull: follow the rename from `_tmp` to `_tmp_path`.
`container.pull()` returns an open file handle; the tests called `.read()`
on it without closing, so under `-W error` the GC-time close emitted a
`ResourceWarning` (surfaced by pytest as `PytestUnraisableExceptionWarning`).
Use `with container.pull(...) as baz:` to close deterministically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant