From e054c3fdad20030666ab1759501873fd99f67143 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 25 Sep 2023 16:01:33 +1300 Subject: [PATCH] test: add type hinting to test_lib (#1012) * Stop testing debug level logs. * Ignore the type checking when we are deliberately checking for bad type handling. * Pass a dummy spec rather than None, to avoid type complaints. * Work around an optional argument. Also remove the use of the `logassert` module. --- ops/lib/__init__.py | 5 +- pyproject.toml | 1 + requirements-dev.txt | 1 - test/test_framework.py | 51 +++++++++--------- test/test_lib.py | 117 ++++++++++++++++------------------------- test/test_main.py | 8 --- 6 files changed, 74 insertions(+), 109 deletions(-) diff --git a/ops/lib/__init__.py b/ops/lib/__init__.py index f67fbd831..2d201256d 100644 --- a/ops/lib/__init__.py +++ b/ops/lib/__init__.py @@ -23,6 +23,7 @@ import os import re import sys +import typing import warnings from ast import literal_eval from importlib.machinery import ModuleSpec @@ -117,7 +118,7 @@ def autoimport(): versions.sort(reverse=True) -def _find_all_specs(path): +def _find_all_specs(path: typing.Iterable[str]) -> typing.Iterator[ModuleSpec]: for sys_dir in path: if sys_dir == "": sys_dir = "." @@ -192,7 +193,7 @@ def __str__(self): return f"got {_join_and(sorted(got))}, but missing {_join_and(sorted(exp - got))}" -def _parse_lib(spec): +def _parse_lib(spec: ModuleSpec) -> typing.Optional["_Lib"]: if spec.origin is None: # "can't happen" logger.warning("No origin for %r (no idea why; please report)", spec.name) diff --git a/pyproject.toml b/pyproject.toml index d4d6b2d22..197b45dfc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,6 +33,7 @@ include = ["ops/*.py", "ops/_private/*.py", "test/test_infra.py", "test/test_jujuversion.py", "test/test_log.py", + "test/test_lib.py", ] pythonVersion = "3.8" # check no python > 3.8 features are used pythonPlatform = "All" diff --git a/requirements-dev.txt b/requirements-dev.txt index 0e2df6322..25bc5a85c 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,5 +1,4 @@ isort==5.11.4 -logassert==7 autopep8==1.6.0 flake8==4.0.1 flake8-docstrings==1.6.0 diff --git a/test/test_framework.py b/test/test_framework.py index 950f5e4db..26f2d89f9 100755 --- a/test/test_framework.py +++ b/test/test_framework.py @@ -25,8 +25,6 @@ from test.test_helpers import BaseTestCase, fake_script from unittest.mock import patch -import logassert - import ops from ops.framework import _BREAKPOINT_WELCOME_MESSAGE, _event_regex from ops.storage import NoSnapshotError, SQLiteStorage @@ -41,13 +39,14 @@ def setUp(self): patcher = patch('ops.storage.SQLiteStorage.DB_LOCK_TIMEOUT', datetime.timedelta(0)) patcher.start() self.addCleanup(patcher.stop) - logassert.setup(self, 'ops') def test_deprecated_init(self): # For 0.7, this still works, but it is deprecated. - framework = ops.Framework(':memory:', None, None, None) - self.assertLoggedWarning( - "deprecated: Framework now takes a Storage not a path") + with self.assertLogs(level="WARNING") as cm: + framework = ops.Framework(':memory:', None, None, None) + self.assertIn( + "WARNING:ops.framework:deprecated: Framework now takes a Storage not a path", + cm.output) self.assertIsInstance(framework._storage, SQLiteStorage) def test_handle_path(self): @@ -356,12 +355,7 @@ def _on_foo(self, event): obs = MyObserver(framework, "1") framework.observe(pub.foo, obs._on_foo) - - self.assertNotLogged("Deferring") pub.foo.emit(1) - self.assertLogged("Deferring .") - self.assertNotLogged("Re-emitting") - framework.reemit() # Two things being checked here: @@ -375,7 +369,6 @@ def _on_foo(self, event): # we'd get a foo=3). # self.assertEqual(obs.seen, ["on_foo:foo=2", "on_foo:foo=2"]) - self.assertLoggedDebug("Re-emitting deferred event .") def test_weak_observer(self): framework = self.create_framework() @@ -1487,10 +1480,6 @@ def callback_method(self, event): @patch('sys.stderr', new_callable=io.StringIO) class BreakpointTests(BaseTestCase): - def setUp(self): - super().setUp() - logassert.setup(self, 'ops') - def test_ignored(self, fake_stderr): # It doesn't do anything really unless proper environment is there. with patch.dict(os.environ): @@ -1498,10 +1487,17 @@ def test_ignored(self, fake_stderr): framework = self.create_framework() with patch('pdb.Pdb.set_trace') as mock: - framework.breakpoint() + # We want to verify that there are *no* logs at warning level. + # However, assertNoLogs is Python 3.10+. + try: + with self.assertLogs(level="WARNING"): + framework.breakpoint() + except AssertionError: + pass + else: + self.fail("No warning logs should be generated") self.assertEqual(mock.call_count, 0) self.assertEqual(fake_stderr.getvalue(), "") - self.assertNotLoggedWarning("Breakpoint", "skipped") def test_pdb_properly_called(self, fake_stderr): # The debugger needs to leave the user in the frame where the breakpoint is executed, @@ -1658,17 +1654,20 @@ def test_named_indicated_specifically(self, fake_stderr): def test_named_indicated_unnamed(self, fake_stderr): # Some breakpoint was indicated, but the framework call was unnamed - self.check_trace_set('some-breakpoint', None, 0) - self.assertLoggedWarning( - "Breakpoint None skipped", - "not found in the requested breakpoints: {'some-breakpoint'}") + with self.assertLogs(level="WARNING") as cm: + self.check_trace_set('some-breakpoint', None, 0) + self.assertEqual(cm.output, [ + "WARNING:ops.framework:Breakpoint None skipped " + "(not found in the requested breakpoints: {'some-breakpoint'})" + ]) def test_named_indicated_somethingelse(self, fake_stderr): # Some breakpoint was indicated, but the framework call was with a different name - self.check_trace_set('some-breakpoint', 'other-name', 0) - self.assertLoggedWarning( - "Breakpoint 'other-name' skipped", - "not found in the requested breakpoints: {'some-breakpoint'}") + with self.assertLogs(level="WARNING") as cm: + self.check_trace_set('some-breakpoint', 'other-name', 0) + self.assertEqual(cm.output, [ + "WARNING:ops.framework:Breakpoint 'other-name' skipped " + "(not found in the requested breakpoints: {'some-breakpoint'})"]) def test_named_indicated_ingroup(self, fake_stderr): # A multiple breakpoint was indicated, and the framework call used a name among those. diff --git a/test/test_lib.py b/test/test_lib.py index 5a23c1f8c..f19aa2ec1 100644 --- a/test/test_lib.py +++ b/test/test_lib.py @@ -14,6 +14,7 @@ import os import sys +import typing from importlib.machinery import ModuleSpec from pathlib import Path from random import shuffle @@ -23,13 +24,17 @@ from unittest import TestCase from unittest.mock import patch -import logassert import pytest import ops.lib # Ignore deprecation warnings for this module. -pytestmark = pytest.mark.filterwarnings('ignore::DeprecationWarning') +pytestmark: pytest.MarkDecorator = pytest.mark.filterwarnings('ignore::DeprecationWarning') + + +# ModuleSpec to pass when we know it will not be used but we want the +# type to match. +_dummy_spec = ModuleSpec("", loader=None) def _mklib(topdir: str, pkgname: str, libname: str) -> Path: @@ -60,14 +65,12 @@ def _mklib(topdir: str, pkgname: str, libname: str) -> Path: return lib / '__init__.py' -def _flatten(specgen): - return sorted([os.path.dirname(spec.origin) for spec in specgen]) +def _flatten(specgen: typing.Iterable[ModuleSpec]) -> typing.List[str]: + return sorted([os.path.dirname(spec.origin if spec.origin is not None else "") + for spec in specgen]) class TestLibFinder(TestCase): - def setUp(self): - logassert.setup(self, 'ops.lib') - def _mkdtemp(self) -> str: tmpdir = mkdtemp() self.addCleanup(rmtree, tmpdir) @@ -77,14 +80,12 @@ def test_single(self): tmpdir = self._mkdtemp() self.assertEqual(list(ops.lib._find_all_specs([tmpdir])), []) - self.assertLoggedDebug('Looking for ops.lib packages under', tmpdir) _mklib(tmpdir, "foo", "bar").write_text("") self.assertEqual( _flatten(ops.lib._find_all_specs([tmpdir])), [os.path.join(tmpdir, 'foo', 'opslib', 'bar')]) - self.assertLoggedDebug("Found", "foo.opslib.bar") def test_multi(self): tmp_dir_a = self._mkdtemp() @@ -173,7 +174,7 @@ def test_namespace(self): class TestLibParser(TestCase): - def _mkmod(self, name: str, content: str = None) -> ModuleSpec: + def _mkmod(self, name: str, content: typing.Optional[str] = None) -> ModuleSpec: fd, fname = mkstemp(text=True) self.addCleanup(os.unlink, fname) if content is not None: @@ -182,9 +183,6 @@ def _mkmod(self, name: str, content: str = None) -> ModuleSpec: os.close(fd) return ModuleSpec(name=name, loader=None, origin=fname) - def setUp(self): - logassert.setup(self, 'ops.lib') - def test_simple(self): """Check that we can load a reasonably straightforward lib.""" m = self._mkmod('foo', ''' @@ -196,11 +194,9 @@ def test_simple(self): LIBANANA = True ''') lib = ops.lib._parse_lib(m) - self.assertEqual(lib, ops.lib._Lib(None, "foo", "alice@example.com", 2, 42)) + self.assertEqual(lib, ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 2, 42)) # also check the repr while we're at it self.assertEqual(repr(lib), '<_Lib foo by alice@example.com, API 2, patch 42>') - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Success") def test_libauthor_has_dashes(self): m = self._mkmod('foo', ''' @@ -211,7 +207,7 @@ def test_libauthor_has_dashes(self): LIBANANA = True ''') lib = ops.lib._parse_lib(m) - self.assertEqual(lib, ops.lib._Lib(None, "foo", "alice-someone@example.com", 2, 42)) + self.assertEqual(lib, ops.lib._Lib(_dummy_spec, "foo", "alice-someone@example.com", 2, 42)) # also check the repr while we're at it self.assertEqual(repr(lib), '<_Lib foo by alice-someone@example.com, API 2, patch 42>') @@ -224,7 +220,7 @@ def test_lib_definitions_without_spaces(self): LIBANANA=True ''') lib = ops.lib._parse_lib(m) - self.assertEqual(lib, ops.lib._Lib(None, "foo", "alice@example.com", 2, 42)) + self.assertEqual(lib, ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 2, 42)) # also check the repr while we're at it self.assertEqual(repr(lib), '<_Lib foo by alice@example.com, API 2, patch 42>') @@ -237,7 +233,7 @@ def test_lib_definitions_trailing_comments(self): LIBANANA = True ''') lib = ops.lib._parse_lib(m) - self.assertEqual(lib, ops.lib._Lib(None, "foo", "alice@example.com", 2, 42)) + self.assertEqual(lib, ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 2, 42)) # also check the repr while we're at it self.assertEqual(repr(lib), '<_Lib foo by alice@example.com, API 2, patch 42>') @@ -249,11 +245,6 @@ def test_incomplete(self): LIBPATCH = 42 ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug( - "Missing opslib metadata after reading to end of file:" - " got API, NAME, and PATCH, but missing AUTHOR") - self.assertNotLogged("Success") def test_too_long(self): """Check that if the file is too long, nothing is returned.""" @@ -264,11 +255,6 @@ def test_too_long(self): LIBAUTHOR = "alice@example.com" ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug( - "Missing opslib metadata after reading to line 99:" - " missing API, AUTHOR, NAME, and PATCH") - self.assertNotLogged("Success") def test_no_origin(self): """Check that _parse_lib doesn't choke when given a spec with no origin.""" @@ -292,9 +278,6 @@ def test_bogus_lib(self): LIBAUTHOR = "alice@example.com" ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Failed") - self.assertNotLogged("Success") def test_name_is_number(self): """Check our behaviour when the name in the lib is a number.""" @@ -305,9 +288,6 @@ def test_name_is_number(self): LIBAUTHOR = "alice@example.com" ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Bad type for NAME: expected str, got int") - self.assertNotLogged("Success") def test_api_is_string(self): """Check our behaviour when the api in the lib is a string.""" @@ -318,9 +298,6 @@ def test_api_is_string(self): LIBAUTHOR = "alice@example.com" ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Bad type for API: expected int, got str") - self.assertNotLogged("Success") def test_patch_is_string(self): """Check our behaviour when the patch in the lib is a string.""" @@ -331,9 +308,6 @@ def test_patch_is_string(self): LIBAUTHOR = "alice@example.com" ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Bad type for PATCH: expected int, got str") - self.assertNotLogged("Success") def test_author_is_number(self): """Check our behaviour when the author in the lib is a number.""" @@ -344,13 +318,15 @@ def test_author_is_number(self): LIBAUTHOR = 43 ''') self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Bad type for AUTHOR: expected str, got int") - self.assertNotLogged("Success") def test_other_encoding(self): """Check that we don't crash when a library is not UTF-8.""" m = self._mkmod('foo') + # This should never be the case, but we need to show type checkers + # that it's not. + if m.origin is None: + self.assertIsNotNone(m.origin) + return with open(m.origin, 'wt', encoding='latin-1') as f: f.write(dedent(''' LIBNAME = "foo" @@ -360,51 +336,48 @@ def test_other_encoding(self): LIBANANA = "Ñoño" ''')) self.assertIsNone(ops.lib._parse_lib(m)) - self.assertLoggedDebug("Parsing", "foo") - self.assertLoggedDebug("Failed", "can't decode") - self.assertNotLogged("Success") class TestLib(TestCase): def test_lib_comparison(self): self.assertNotEqual( - ops.lib._Lib(None, "foo", "alice@example.com", 1, 0), - ops.lib._Lib(None, "bar", "bob@example.com", 0, 1)) + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 0), + ops.lib._Lib(_dummy_spec, "bar", "bob@example.com", 0, 1)) self.assertEqual( - ops.lib._Lib(None, "foo", "alice@example.com", 1, 1), - ops.lib._Lib(None, "foo", "alice@example.com", 1, 1)) + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1), + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1)) self.assertLess( - ops.lib._Lib(None, "foo", "alice@example.com", 1, 0), - ops.lib._Lib(None, "foo", "alice@example.com", 1, 1)) + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 0), + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1)) self.assertLess( - ops.lib._Lib(None, "foo", "alice@example.com", 0, 1), - ops.lib._Lib(None, "foo", "alice@example.com", 1, 1)) + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 0, 1), + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1)) self.assertLess( - ops.lib._Lib(None, "foo", "alice@example.com", 1, 1), - ops.lib._Lib(None, "foo", "bob@example.com", 1, 1)) + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1), + ops.lib._Lib(_dummy_spec, "foo", "bob@example.com", 1, 1)) self.assertLess( - ops.lib._Lib(None, "bar", "alice@example.com", 1, 1), - ops.lib._Lib(None, "foo", "alice@example.com", 1, 1)) + ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1), + ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1)) with self.assertRaises(TypeError): - 42 < ops.lib._Lib(None, "bar", "alice@example.com", 1, 1) + 42 < ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) # type:ignore with self.assertRaises(TypeError): - ops.lib._Lib(None, "bar", "alice@example.com", 1, 1) < 42 + ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) < 42 # type: ignore # these two might be surprising in that they don't raise an exception, # but they are correct: our __eq__ bailing means Python falls back to # its default of checking object identity. - self.assertNotEqual(ops.lib._Lib(None, "bar", "alice@example.com", 1, 1), 42) - self.assertNotEqual(42, ops.lib._Lib(None, "bar", "alice@example.com", 1, 1)) + self.assertNotEqual(ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1), 42) + self.assertNotEqual(42, ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1)) def test_lib_order(self): - a = ops.lib._Lib(None, "bar", "alice@example.com", 1, 0) - b = ops.lib._Lib(None, "bar", "alice@example.com", 1, 1) - c = ops.lib._Lib(None, "foo", "alice@example.com", 1, 0) - d = ops.lib._Lib(None, "foo", "alice@example.com", 1, 1) - e = ops.lib._Lib(None, "foo", "bob@example.com", 1, 1) + a = ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 0) + b = ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) + c = ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 0) + d = ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1) + e = ops.lib._Lib(_dummy_spec, "foo", "bob@example.com", 1, 1) for i in range(20): with self.subTest(i): @@ -414,11 +387,11 @@ def test_lib_order(self): def test_use_bad_args_types(self): with self.assertRaises(TypeError): - ops.lib.use(1, 2, 'bob@example.com') + ops.lib.use(1, 2, 'bob@example.com') # type: ignore with self.assertRaises(TypeError): - ops.lib.use('foo', '2', 'bob@example.com') + ops.lib.use('foo', '2', 'bob@example.com') # type: ignore with self.assertRaises(TypeError): - ops.lib.use('foo', 2, ops.lib.use) + ops.lib.use('foo', 2, ops.lib.use) # type: ignore def test_use_bad_args_values(self): with self.assertRaises(ValueError): @@ -559,7 +532,7 @@ def test_from_scratch(self): baz = ops.lib.use('baz', 2, 'alice@example.com') self.assertEqual(baz.LIBAPI, 2) - def _test_submodule(self, *, relative=False): + def _test_submodule(self, *, relative: bool = False): tmpdir = self._mkdtemp() sys.path = [tmpdir] diff --git a/test/test_main.py b/test/test_main.py index 25c0ba286..49d8a604a 100755 --- a/test/test_main.py +++ b/test/test_main.py @@ -27,8 +27,6 @@ from pathlib import Path from unittest.mock import patch -import logassert - import ops from ops.main import CHARM_STATE_FILE, _should_use_controller_storage from ops.storage import SQLiteStorage @@ -1137,26 +1135,20 @@ def _call_event(self, rel_path, env): class TestStorageHeuristics(unittest.TestCase): - def setUp(self): - logassert.setup(self, '') - def test_fallback_to_current_juju_version__too_old(self): meta = ops.CharmMeta.from_yaml("series: [kubernetes]") with patch.dict(os.environ, {"JUJU_VERSION": "1.0"}): self.assertFalse(_should_use_controller_storage(Path("/xyzzy"), meta)) - self.assertLogged('Using local storage: JUJU_VERSION=1.0.0') def test_fallback_to_current_juju_version__new_enough(self): meta = ops.CharmMeta.from_yaml("series: [kubernetes]") with patch.dict(os.environ, {"JUJU_VERSION": "2.8"}): self.assertTrue(_should_use_controller_storage(Path("/xyzzy"), meta)) - self.assertLogged('Using controller storage: JUJU_VERSION=2.8.0') def test_not_if_not_in_k8s(self): meta = ops.CharmMeta.from_yaml("series: [ecs]") with patch.dict(os.environ, {"JUJU_VERSION": "2.8"}): self.assertFalse(_should_use_controller_storage(Path("/xyzzy"), meta)) - self.assertLogged('Using local storage: not a Kubernetes podspec charm') def test_not_if_already_local(self): meta = ops.CharmMeta.from_yaml("series: [kubernetes]")