From 7ea0f154eb9157b958c95763e5a4a9eaaf219496 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Thu, 21 Dec 2023 22:01:12 -0500 Subject: [PATCH 1/3] RF: centralize invocation of coverage within run_coverage and use sys.executable -m coverage - duplication is evil - some systems might have it installed as python3-coverage not as coverage (Debian) - executable "coverage" might use shebang pointing to another python than used here Originally crafted for Debian package --- tests/validation/test_validate.py | 84 ++++++++++++------------------- 1 file changed, 33 insertions(+), 51 deletions(-) diff --git a/tests/validation/test_validate.py b/tests/validation/test_validate.py index 6aa2ee25e..557596f51 100644 --- a/tests/validation/test_validate.py +++ b/tests/validation/test_validate.py @@ -1,5 +1,6 @@ import subprocess import re +import sys from unittest.mock import patch from io import StringIO import warnings @@ -8,26 +9,35 @@ from pynwb import validate, NWBHDF5IO +# NOTE we use "coverage run -m pynwb.validate" instead of "python -m pynwb.validate" +# so that we can both test pynwb.validate and compute code coverage from that test. +# NOTE we also use "coverage run -p" which will generate a .coverage file with the +# machine name, process id, and a random number appended to the filename to +# simplify collecting and merging coverage data from multiple subprocesses. if "-p" +# is not used, then each "coverage run" will overwrite the .coverage file from a +# previous "coverage run". +# NOTE we run "coverage" as "{sys.executable} -m coverage" to 1. make sure to use +# the same python version, and on Debian systems executable is "python3-coverage", not +# just "coverage". +# NOTE the run_coverage.yml GitHub Action runs "python -m coverage combine" to +# combine the individual coverage reports into one .coverage file. +def run_coverage(extra_args: list[str]): + return subprocess.run( + [sys.executable, "-m", "coverage", "run", "-p", "-m", "pynwb.validate"] + + extra_args, + capture_output=True + ) + + class TestValidateCLI(TestCase): # 1.0.2_nwbfile.nwb has no cached specifications # 1.0.3_nwbfile.nwb has cached "core" specification # 1.1.2_nwbfile.nwb has cached "core" and "hdmf-common" specifications - # NOTE we use "coverage run -m pynwb.validate" instead of "python -m pynwb.validate" - # so that we can both test pynwb.validate and compute code coverage from that test. - # NOTE we also use "coverage run -p" which will generate a .coverage file with the - # machine name, process id, and a random number appended to the filename to - # simplify collecting and merging coverage data from multiple subprocesses. if "-p" - # is not used, then each "coverage run" will overwrite the .coverage file from a - # previous "coverage run". - # NOTE the run_coverage.yml GitHub Action runs "python -m coverage combine" to - # combine the individual coverage reports into one .coverage file. - def test_validate_file_no_cache(self): """Test that validating a file with no cached spec against the core namespace succeeds.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", - "tests/back_compat/1.0.2_nwbfile.nwb"], capture_output=True) + result = run_coverage(["tests/back_compat/1.0.2_nwbfile.nwb"]) stderr_regex = re.compile( r"The file tests/back_compat/1\.0\.2_nwbfile\.nwb has no cached namespace information\. " @@ -42,8 +52,7 @@ def test_validate_file_no_cache(self): def test_validate_file_no_cache_bad_ns(self): """Test that validating a file with no cached spec against a specified, unknown namespace fails.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", "tests/back_compat/1.0.2_nwbfile.nwb", - "--ns", "notfound"], capture_output=True) + result = run_coverage(["tests/back_compat/1.0.2_nwbfile.nwb", "--ns", "notfound"]) stderr_regex = re.compile( r"The file tests/back_compat/1\.0\.2_nwbfile\.nwb has no cached namespace information\. " @@ -57,8 +66,7 @@ def test_validate_file_no_cache_bad_ns(self): def test_validate_file_cached(self): """Test that validating a file with cached spec against its cached namespace succeeds.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", - "tests/back_compat/1.1.2_nwbfile.nwb"], capture_output=True) + result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb"]) self.assertEqual(result.stderr.decode('utf-8'), '') @@ -69,8 +77,7 @@ def test_validate_file_cached(self): def test_validate_file_cached_bad_ns(self): """Test that validating a file with cached spec against a specified, unknown namespace fails.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", - "tests/back_compat/1.1.2_nwbfile.nwb", "--ns", "notfound"], capture_output=True) + result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb", "--ns", "notfound"]) stderr_regex = re.compile( r"The namespace 'notfound' could not be found in cached namespace information as only " @@ -82,8 +89,7 @@ def test_validate_file_cached_bad_ns(self): def test_validate_file_cached_extension(self): """Test that validating a file with cached spec against the cached namespaces succeeds.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", - "tests/back_compat/2.1.0_nwbfile_with_extension.nwb"], capture_output=True) + result = run_coverage(["tests/back_compat/2.1.0_nwbfile_with_extension.nwb"]) self.assertEqual(result.stderr.decode('utf-8'), '') @@ -94,9 +100,7 @@ def test_validate_file_cached_extension(self): def test_validate_file_cached_extension_pass_ns(self): """Test that validating a file with cached spec against the extension namespace succeeds.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", - "tests/back_compat/2.1.0_nwbfile_with_extension.nwb", - "--ns", "ndx-testextension"], capture_output=True) + result = run_coverage(["tests/back_compat/2.1.0_nwbfile_with_extension.nwb", "--ns", "ndx-testextension"]) self.assertEqual(result.stderr.decode('utf-8'), '') @@ -107,9 +111,7 @@ def test_validate_file_cached_extension_pass_ns(self): def test_validate_file_cached_core(self): """Test that validating a file with cached spec against the core namespace succeeds.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", - "tests/back_compat/2.1.0_nwbfile_with_extension.nwb", - "--ns", "core"], capture_output=True) + result = run_coverage(["tests/back_compat/2.1.0_nwbfile_with_extension.nwb", "--ns", "core"]) stdout_regex = re.compile( r"The namespace 'core' is included by the namespace 'ndx-testextension'. " @@ -119,8 +121,7 @@ def test_validate_file_cached_core(self): def test_validate_file_cached_hdmf_common(self): """Test that validating a file with cached spec against the hdmf-common namespace fails.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", "tests/back_compat/1.1.2_nwbfile.nwb", - "--ns", "hdmf-common"], capture_output=True) + result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb", "--ns", "hdmf-common"]) stderr_regex = re.compile( r"The namespace 'hdmf-common' is included by the namespace 'core'\. Please validate against that " @@ -130,8 +131,7 @@ def test_validate_file_cached_hdmf_common(self): def test_validate_file_cached_ignore(self): """Test that validating a file with cached spec against the core namespace succeeds.""" - result = subprocess.run(["coverage", "run", "-p", "-m", "pynwb.validate", "tests/back_compat/1.1.2_nwbfile.nwb", - "--no-cached-namespace"], capture_output=True) + result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb", "--no-cached-namespace"] ) self.assertEqual(result.stderr.decode('utf-8'), '') @@ -142,13 +142,7 @@ def test_validate_file_cached_ignore(self): def test_validate_file_invalid(self): """Test that validating an invalid file outputs errors.""" - result = subprocess.run( - [ - "coverage", "run", "-p", "-m", "pynwb.validate", "tests/back_compat/1.0.2_str_experimenter.nwb", - "--no-cached-namespace" - ], - capture_output=True - ) + result = run_coverage(["tests/back_compat/1.0.2_str_experimenter.nwb", "--no-cached-namespace"]) stderr_regex = re.compile( r" - found the following errors:\s*" @@ -164,13 +158,7 @@ def test_validate_file_invalid(self): def test_validate_file_list_namespaces_core(self): """Test listing namespaces from a file""" - result = subprocess.run( - [ - "coverage", "run", "-p", "-m", "pynwb.validate", "tests/back_compat/1.1.2_nwbfile.nwb", - "--list-namespaces" - ], - capture_output=True - ) + result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb", "--list-namespaces"]) self.assertEqual(result.stderr.decode('utf-8'), '') @@ -179,13 +167,7 @@ def test_validate_file_list_namespaces_core(self): def test_validate_file_list_namespaces_extension(self): """Test listing namespaces from a file with an extension""" - result = subprocess.run( - [ - "coverage", "run", "-p", "-m", "pynwb.validate", "tests/back_compat/2.1.0_nwbfile_with_extension.nwb", - "--list-namespaces" - ], - capture_output=True - ) + result = run_coverage(["tests/back_compat/2.1.0_nwbfile_with_extension.nwb", "--list-namespaces"]) self.assertEqual(result.stderr.decode('utf-8'), '') From bd357a5d9c6173ad55d4b152e3be93f1081df849 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 21 Dec 2023 20:57:13 -0800 Subject: [PATCH 2/3] Update tests/validation/test_validate.py --- tests/validation/test_validate.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/validation/test_validate.py b/tests/validation/test_validate.py index 557596f51..c2829ee1f 100644 --- a/tests/validation/test_validate.py +++ b/tests/validation/test_validate.py @@ -131,7 +131,7 @@ def test_validate_file_cached_hdmf_common(self): def test_validate_file_cached_ignore(self): """Test that validating a file with cached spec against the core namespace succeeds.""" - result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb", "--no-cached-namespace"] ) + result = run_coverage(["tests/back_compat/1.1.2_nwbfile.nwb", "--no-cached-namespace"]) self.assertEqual(result.stderr.decode('utf-8'), '') From 73d71b6294e16a2303d284d9cd01ced20ac80d45 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 21 Dec 2023 21:06:16 -0800 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f38f6001d..c5cfa5a9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - Expose the offset, conversion and channel conversion parameters in `mock_ElectricalSeries`. @h-mayorquin [#1796](https://github.com/NeurodataWithoutBorders/pynwb/pull/1796) - Expose `starting_time` in `mock_ElectricalSeries`. @h-mayorquin [#1805](https://github.com/NeurodataWithoutBorders/pynwb/pull/1805) - Enhance `get_data_in_units()` to work with objects that have a `channel_conversion` attribute like the `ElectricalSeries`. @h-mayorquin [#1806](https://github.com/NeurodataWithoutBorders/pynwb/pull/1806) +- Refactor validation CLI tests to use `{sys.executable} -m coverage` to use the same Python version and run correctly on Debian systems. @yarikoptic [#1811](https://github.com/NeurodataWithoutBorders/pynwb/pull/1811) ### Bug fixes - Fix bug where namespaces were loaded in "w-" mode. @h-mayorquin [#1795](https://github.com/NeurodataWithoutBorders/pynwb/pull/1795)