From 36da3f724b6168983722aac1edf0c1f34b54e666 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Wed, 21 Oct 2026 11:07:42 +0200 Subject: [PATCH 1/8] [ENH] Add ELEPHANT_DATA_ROOT_URL and expand dataset download tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce ELEPHANT_DATA_ROOT_URL constant for the default dataset URL Use it in download_datasets to allow test‑time patching Add tests with a local HTTP server covering versioned dataset download, integrity checks and fallback warning behavior --- elephant/datasets.py | 10 ++- elephant/test/test_datasets.py | 140 ++++++++++++++++++++++++++++----- 2 files changed, 127 insertions(+), 23 deletions(-) diff --git a/elephant/datasets.py b/elephant/datasets.py index d03b1874a..522c94a97 100644 --- a/elephant/datasets.py +++ b/elephant/datasets.py @@ -89,6 +89,10 @@ ELEPHANT_TMP_DIR = Path(tempfile.gettempdir()) / "elephant" +# Default URL of `elephant-data`, used when `ELEPHANT_DATA_LOCATION` not set. +# This redirects to the current location of `elephant-data` +ELEPHANT_DATA_ROOT_URL = "https://datasets.python-elephant.org/" + # Data generation functions @@ -395,9 +399,9 @@ def download_datasets(repo_path, filepath=None, checksum=None, else: # The user did not set a URL or path in `ELEPHANT_DATA_LOCATION`. - # Use the default root URL, which redirects to the current location of - # `elephant-data`. - url_to_root = "https://datasets.python-elephant.org/" + # Use the default root URL. + # Read from module at call time, so that tests can patch it + url_to_root = ELEPHANT_DATA_ROOT_URL # Get the final URL to the current version of `elephant data` # (version of Elephant is equal to version of `elephant-data`). diff --git a/elephant/test/test_datasets.py b/elephant/test/test_datasets.py index 0817a01ae..ee38e24f0 100644 --- a/elephant/test/test_datasets.py +++ b/elephant/test/test_datasets.py @@ -1,10 +1,14 @@ import unittest import os +import threading +from functools import partial +from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer from unittest.mock import patch from pathlib import Path from tempfile import TemporaryDirectory, gettempdir import hashlib -from urllib.error import URLError +import warnings +from urllib.error import HTTPError, URLError import numpy as np from numpy.testing import assert_allclose @@ -22,10 +26,69 @@ and Path(os.environ["ELEPHANT_DATA_LOCATION"]).is_dir()) +class _QuietHandler(SimpleHTTPRequestHandler): + # To avoid logging each request to stderr + def log_message(self, format, *args): + pass + + class TestDownloadDatasets(unittest.TestCase): - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': '/invalid/path'}, - clear=True) + # Version served under `raw/v{FAKE_VERSION}`, for the version probe tests + FAKE_VERSION = '9.9.9' + + @classmethod + def setUpClass(cls): + """ + Serve a minimal copy of `elephant-data` from 127.0.0.1 + so tests don't have to contact GIN. The master branch + and one fake version of the GIN dataset are mocked. + """ + # Mockup a dummy dataset location + cls._serve_dir = TemporaryDirectory() + raw = Path(cls._serve_dir.name) / 'raw' + cls.repo_data_path = 'dataset-1/dataset-1.h5' + + # Dummy dataset, content is irrelevant for tests + cls.master_payload = bytes(range(256)) * 4 + dataset = raw / 'master' / cls.repo_data_path + dataset.parent.mkdir(parents=True) + dataset.write_bytes(cls.master_payload) + + # Versioned copy with different content, so tests can tell which one + # was downloaded. `download_datasets` probes the README of the + # version, so only the versioned copy needs one. + cls.versioned_payload = b'versioned' * 100 + versioned = raw / f'v{cls.FAKE_VERSION}' + dataset = versioned / cls.repo_data_path + dataset.parent.mkdir(parents=True) + dataset.write_bytes(cls.versioned_payload) + (versioned / 'README.md').write_text('Mock elephant-data\n') + + # Port 0 lets the OS pick a free port + handler = partial(_QuietHandler, directory=cls._serve_dir.name) + cls.server = ThreadingHTTPServer(('127.0.0.1', 0), handler) + cls.root_url = f'http://127.0.0.1:{cls.server.server_port}/' + cls._server_thread = threading.Thread(target=cls.server.serve_forever, + daemon=True) + cls._server_thread.start() + + @classmethod + def tearDownClass(cls): + cls.server.shutdown() + cls.server.server_close() + cls._server_thread.join() + cls._serve_dir.cleanup() + + def setUp(self): + # Point the default root URL (used when `ELEPHANT_DATA_LOCATION` is + # empty) at the local server, so no test in this class contacts GIN. + patcher = patch('elephant.datasets.ELEPHANT_DATA_ROOT_URL', + self.root_url) + patcher.start() + self.addCleanup(patcher.stop) + + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': '/invalid/path'}) def test_invalid_path(self): repo_path = 'some/repo/path' with self.assertRaises(ValueError) as error: @@ -35,8 +98,7 @@ def test_invalid_path(self): exception_msg) self.assertIn("/invalid/path", exception_msg) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': 'ftp://invalid.url'}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': 'ftp://invalid.url'}) def test_invalid_url(self): repo_path = 'some/repo/path' with self.assertRaises(ValueError) as error: @@ -46,19 +108,17 @@ def test_invalid_url(self): exception_msg) self.assertIn("ftp://invalid.url", exception_msg) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': 'http://valid.url'}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': 'http://valid.url'}) def test_valid_url(self): repo_path = 'some/repo/path' self.assertRaises(URLError, download_datasets, repo_path) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}, clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) def test_invalid_data(self): repo_path = 'some/repo/path' self.assertRaises(URLError, download_datasets, repo_path) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': 'invalid_path_or_url'}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': 'invalid_path_or_url'}) def test_invalid_value(self): repo_path = 'some/repo/path' with self.assertRaises(ValueError) as error: @@ -68,8 +128,7 @@ def test_invalid_value(self): exception_msg) self.assertIn("invalid_path_or_url", exception_msg) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) def test_valid_data(self): # This is expected to download a file with the same name in GIN to a # folder 'elephant' within the current system temporary folder @@ -84,8 +143,7 @@ def test_valid_data(self): self.assertTrue(Path(downloaded_file).is_file()) self.assertEqual(expected_file_path, downloaded_file) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) def test_valid_data_with_path(self): # This is expected to download a file to the provided path # (using a temporary directory specific to the test) @@ -98,8 +156,7 @@ def test_valid_data_with_path(self): self.assertTrue(Path(downloaded_file).is_file()) self.assertEqual(target_file_path, downloaded_file) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) def test_valid_data_with_integrity_check(self): # This is expected to check the integrity of the downloaded file by # comparing the MD5 hash. We manually download the file to the current @@ -124,8 +181,7 @@ def test_valid_data_with_integrity_check(self): self.assertTrue(Path(downloaded_file).is_file()) self.assertEqual(target_file_path, downloaded_file) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) def test_valid_data_existing(self): # This is expected to avoid downloading the file two times, since # the file has been previously downloaded and a checksum is provided. @@ -148,8 +204,7 @@ def test_valid_data_existing(self): self.assertEqual(target_file_path, existing_file) self.assertEqual(downloaded_file, existing_file) - @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}, - clear=True) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) def test_valid_data_with_failed_integrity_check(self): # This forces a failure by setting an invalid checksum for a dataset # in GIN. @@ -166,6 +221,50 @@ def test_valid_data_with_failed_integrity_check(self): self.assertIn("Data at", exception_msg) self.assertIn("does not agree with MD5 hash aaaaaa", exception_msg) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) + def test_valid_data_versioned(self): + """ + Intent: Ensure versioned dataset download path works + Condition: Version of Elephant exists in (mocked) `elephant-data` + Expected: No warning raised + """ + with ( + patch('elephant.datasets._get_version', + return_value=self.FAKE_VERSION), + TemporaryDirectory() as temp_dir, + warnings.catch_warnings(record=True) as caught, + ): + warnings.simplefilter('always') + target_file_path = Path(temp_dir) / 'target_versioned' + downloaded_file = download_datasets(self.repo_data_path, + filepath=target_file_path) + self.assertEqual(target_file_path, downloaded_file) + self.assertEqual(self.versioned_payload, + Path(downloaded_file).read_bytes()) + self.assertFalse([w for w in caught + if issubclass(w.category, UserWarning)]) + + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) + def test_valid_data_version_fallback(self): + """ + Intent: Ensure fallback to master branch of dataset if version not + available + Condition: Version of Elephant does not exist in `elephant-data` (404) + Expected: Raises a warning + """ + with ( + patch('elephant.datasets._get_version', return_value='0.0.0'), + TemporaryDirectory() as temp_dir, + ): + target_file_path = Path(temp_dir) / 'target_fallback' + with self.assertWarns(UserWarning) as warning: + downloaded_file = download_datasets(self.repo_data_path, + filepath=target_file_path) + self.assertIn("No corresponding version", str(warning.warning)) + self.assertEqual(target_file_path, downloaded_file) + self.assertEqual(self.master_payload, + Path(downloaded_file).read_bytes()) + @unittest.skipIf(not HAS_CACHE_DIR, "Not testing since ELEPHANT_DATA_LOCATION is not set to " "a folder") @@ -290,6 +389,7 @@ def test_download_with_cache_dir_invalid_file(self): self.assertIn(f"'{repo_path}' does not exist in that path", exception_msg) + class TestLoadData(unittest.TestCase): def test_load_data_invalid(self): From b157d7d18c09a24648c91801f1ae287db2dd52b0 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Wed, 21 Oct 2026 11:14:26 +0200 Subject: [PATCH 2/8] [ENH] Add optional GIN connectivity tests and CI step --- .github/workflows/canary_ci.yml | 10 +++++ elephant/test/test_datasets.py | 74 ++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/.github/workflows/canary_ci.yml b/.github/workflows/canary_ci.yml index b88424605..57284288a 100644 --- a/.github/workflows/canary_ci.yml +++ b/.github/workflows/canary_ci.yml @@ -79,3 +79,13 @@ jobs: - name: Test with pytest run: pytest + + # Opt-in tests to check network connectivity for elephant-data + # repository on GIN, kept out of regular CI because GIN + # may answer runners with HTTP 403. + - name: Test GIN connectivity + if: ${{ !cancelled() }} + timeout-minutes: 10 + env: + ELEPHANT_NETWORK_TESTS: "1" + run: pytest -v elephant/test/test_datasets.py::TestGinConnectivity diff --git a/elephant/test/test_datasets.py b/elephant/test/test_datasets.py index ee38e24f0..e7c08b681 100644 --- a/elephant/test/test_datasets.py +++ b/elephant/test/test_datasets.py @@ -9,11 +9,14 @@ import hashlib import warnings from urllib.error import HTTPError, URLError +from urllib.parse import urlparse +from urllib.request import urlopen import numpy as np from numpy.testing import assert_allclose import neo -from elephant.datasets import download_datasets, load_data, ELEPHANT_DATA +from elephant.datasets import (download_datasets, load_data, ELEPHANT_DATA, + ELEPHANT_DATA_ROOT_URL) # Most tests are done with mocking the environment variable to test different @@ -25,6 +28,10 @@ HAS_CACHE_DIR = ("ELEPHANT_DATA_LOCATION" in os.environ and Path(os.environ["ELEPHANT_DATA_LOCATION"]).is_dir()) +# Opt in flag for tests that contact the real `elephant-data` repository on GIN +# To run set env val ELEPHANT_NETWORK_TESTS=1. +RUN_NETWORK_TESTS = os.environ.get("ELEPHANT_NETWORK_TESTS") == "1" + class _QuietHandler(SimpleHTTPRequestHandler): # To avoid logging each request to stderr @@ -390,6 +397,71 @@ def test_download_with_cache_dir_invalid_file(self): exception_msg) +@unittest.skipUnless(RUN_NETWORK_TESTS, + "Not testing GIN connectivity since " + "ELEPHANT_NETWORK_TESTS is not set to 1") +class TestGinConnectivity(unittest.TestCase): + """ + Checks that the real `elephant-data` repository on GIN is reachable via + the default root URL. + """ + + GIN_HOST = 'gin.g-node.org' + TIMEOUT = 60 # seconds + repo_data_path = 'dataset-1/dataset-1.h5' + + def _open(self, url): + # Re-raise HTTP errors with the URL, so a 403 from GIN is obvious + # in the CI log + try: + return urlopen(url, timeout=self.TIMEOUT) + except HTTPError as error: + self.fail(f"GET {url} failed with HTTP {error.code}: {error}") + + def test_root_url_redirects_to_gin(self): + """ + Intent: Ensure the default root URL reaches GIN + Condition: Receive README of the master branch requested + Expected: HTTP 200, final URL is on GIN + """ + url = ELEPHANT_DATA_ROOT_URL + 'raw/master/README.md' + with self._open(url) as response: + self.assertEqual(200, response.status) + self.assertEqual(self.GIN_HOST, + urlparse(response.geturl()).hostname) + + def test_missing_version_probe_is_404(self): + """ + Intent: Ensure the version probe of `download_datasets` gets a real + "not found" for a missing version, not e.g. a 403 (Forbidden) + Condition: README of a version that does not exist is requested + Expected: HTTPError with code 404 + """ + url = ELEPHANT_DATA_ROOT_URL + 'raw/v0.0.0/README.md' + with self.assertRaises(HTTPError) as error: + urlopen(url, timeout=self.TIMEOUT) + self.assertEqual(404, error.exception.code) + + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) + def test_download_default_url(self): + """ + Intent: Ensure `download_datasets` works end to end against GIN + Condition: `ELEPHANT_DATA_LOCATION` is empty, so the default root URL + and the version probe are used + Expected: The file is downloaded to the requested path without error + """ + # TODO: Discuss with the team what other checks can be done to verify + # the end-to-end download from GIN (e.g. content/checksum, given that + # the downloaded version may differ from any cached copy). + with TemporaryDirectory() as temp_dir: + target_file_path = Path(temp_dir) / 'dataset-1.h5' + downloaded_file = download_datasets(self.repo_data_path, + filepath=target_file_path, + verbose=False) + self.assertTrue(Path(downloaded_file).is_file()) + self.assertEqual(target_file_path, downloaded_file) + + class TestLoadData(unittest.TestCase): def test_load_data_invalid(self): From 4952a8c2ab2175f3de726a14038aee5555307a9c Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Wed, 28 Oct 2026 14:39:14 +0100 Subject: [PATCH 3/8] [ENH] Add 404 handling for dataset version probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dataset download now only falls back to the master branch when a 404 (missing version) occurs. Other HTTP errors such as 403 are re‑raised, preventing silent fallback. Added tests to verify the behavior and ensure no files are downloaded when a forbidden version is encountered. --- elephant/datasets.py | 6 ++++++ elephant/test/test_datasets.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/elephant/datasets.py b/elephant/datasets.py index 522c94a97..f14365df9 100644 --- a/elephant/datasets.py +++ b/elephant/datasets.py @@ -414,6 +414,9 @@ def download_datasets(repo_path, filepath=None, checksum=None, urlopen(data_location + '/README.md') except HTTPError as error: + # Only missing version (404) should fall back to master. (PR 721) + if error.code != 404: + raise # If the corresponding `elephant-data` version is not found, # use the latest commit of `elephant-data` (`master` branch). # This is expected for development versions of Elephant, which may @@ -434,6 +437,9 @@ def download_datasets(repo_path, filepath=None, checksum=None, ctx.check_hostname = True urlopen(data_location + '/README.md', context=ctx) except HTTPError as unverified_ctx_error: # e.g. 404 + # Only missing version (404) fall back to master. (PR 721) + if unverified_ctx_error.code != 404: + raise # If it still fails, use latest commit of `elephant-data` in # the `master` branch. data_location = url_to_root + "raw/master" diff --git a/elephant/test/test_datasets.py b/elephant/test/test_datasets.py index e7c08b681..7c4d8c62f 100644 --- a/elephant/test/test_datasets.py +++ b/elephant/test/test_datasets.py @@ -32,12 +32,22 @@ # To run set env val ELEPHANT_NETWORK_TESTS=1. RUN_NETWORK_TESTS = os.environ.get("ELEPHANT_NETWORK_TESTS") == "1" +# Version the mock server answers with HTTP 403, like GIN sometimes +# answers GitHub runners +FORBIDDEN_VERSION = '6.6.6' + class _QuietHandler(SimpleHTTPRequestHandler): # To avoid logging each request to stderr def log_message(self, format, *args): pass + def do_GET(self): + if self.path.startswith(f'/raw/v{FORBIDDEN_VERSION}/'): + self.send_error(403) + return + super().do_GET() + class TestDownloadDatasets(unittest.TestCase): @@ -272,6 +282,25 @@ def test_valid_data_version_fallback(self): self.assertEqual(self.master_payload, Path(downloaded_file).read_bytes()) + @patch.dict(os.environ, {'ELEPHANT_DATA_LOCATION': ''}) + def test_valid_data_version_forbidden(self): + """ + Intent: Ensure only a missing version (404) falls back to master + Condition: Version probe of `elephant-data` answers 403 + Expected: HTTPError 403 raised, nothing downloaded from master + """ + with ( + patch('elephant.datasets._get_version', + return_value=FORBIDDEN_VERSION), + TemporaryDirectory() as temp_dir, + ): + target_file_path = Path(temp_dir) / 'target_forbidden' + with self.assertRaises(HTTPError) as error: + download_datasets(self.repo_data_path, + filepath=target_file_path) + self.assertEqual(403, error.exception.code) + self.assertFalse(target_file_path.exists()) + @unittest.skipIf(not HAS_CACHE_DIR, "Not testing since ELEPHANT_DATA_LOCATION is not set to " "a folder") From 702b2f0a7dcd4ff849b59bc6aeb6689eb4f715d9 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Wed, 28 Oct 2026 14:41:12 +0100 Subject: [PATCH 4/8] [ENH] Warn on GIN connectivity failures instead of failing CI --- .github/workflows/canary_ci.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/canary_ci.yml b/.github/workflows/canary_ci.yml index 57284288a..d63777e48 100644 --- a/.github/workflows/canary_ci.yml +++ b/.github/workflows/canary_ci.yml @@ -88,4 +88,8 @@ jobs: timeout-minutes: 10 env: ELEPHANT_NETWORK_TESTS: "1" - run: pytest -v elephant/test/test_datasets.py::TestGinConnectivity + # Warn only: GIN may answer runners with HTTP 403, which is not an + # elephant failure. The warning shows on the run summary. + run: | + pytest -v elephant/test/test_datasets.py::TestGinConnectivity \ + || echo "::warning title=GIN connectivity::GIN connectivity tests failed (e.g. HTTP 403 from GIN). See the step log." From 009be0cab2aebd1864074ac5add620fa1ede1b75 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Wed, 28 Oct 2026 17:24:01 +0100 Subject: [PATCH 5/8] [ENH] Use addClassCleanup for better teardown --- elephant/test/test_datasets.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/elephant/test/test_datasets.py b/elephant/test/test_datasets.py index 7c4d8c62f..02bfcf96d 100644 --- a/elephant/test/test_datasets.py +++ b/elephant/test/test_datasets.py @@ -63,6 +63,7 @@ def setUpClass(cls): """ # Mockup a dummy dataset location cls._serve_dir = TemporaryDirectory() + cls.addClassCleanup(cls._serve_dir.cleanup) raw = Path(cls._serve_dir.name) / 'raw' cls.repo_data_path = 'dataset-1/dataset-1.h5' @@ -85,17 +86,13 @@ def setUpClass(cls): # Port 0 lets the OS pick a free port handler = partial(_QuietHandler, directory=cls._serve_dir.name) cls.server = ThreadingHTTPServer(('127.0.0.1', 0), handler) + cls.addClassCleanup(cls.server.server_close) cls.root_url = f'http://127.0.0.1:{cls.server.server_port}/' cls._server_thread = threading.Thread(target=cls.server.serve_forever, daemon=True) cls._server_thread.start() - - @classmethod - def tearDownClass(cls): - cls.server.shutdown() - cls.server.server_close() - cls._server_thread.join() - cls._serve_dir.cleanup() + cls.addClassCleanup(cls._server_thread.join) + cls.addClassCleanup(cls.server.shutdown) def setUp(self): # Point the default root URL (used when `ELEPHANT_DATA_LOCATION` is From fa2d9bd7c0aa473116571abf57f9c4daf3c8ed24 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Fri, 30 Oct 2026 14:27:36 +0100 Subject: [PATCH 6/8] [CHORE] Remove dead TODO comment from dataset tests --- elephant/test/test_datasets.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/elephant/test/test_datasets.py b/elephant/test/test_datasets.py index 02bfcf96d..64f8a6c5a 100644 --- a/elephant/test/test_datasets.py +++ b/elephant/test/test_datasets.py @@ -476,9 +476,6 @@ def test_download_default_url(self): and the version probe are used Expected: The file is downloaded to the requested path without error """ - # TODO: Discuss with the team what other checks can be done to verify - # the end-to-end download from GIN (e.g. content/checksum, given that - # the downloaded version may differ from any cached copy). with TemporaryDirectory() as temp_dir: target_file_path = Path(temp_dir) / 'dataset-1.h5' downloaded_file = download_datasets(self.repo_data_path, From 374f668214941a320ccd934c0eed3f2965de3d19 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Tue, 3 Nov 2026 09:15:40 +0100 Subject: [PATCH 7/8] [DOC] Add ADR for unit test documentation style Add ADR describing the new Intent/Condition/Expected docstring format and prune the adr directory from the source distribution. --- MANIFEST.in | 1 + doc/adr/0001-test-documentation-style.md | 54 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 doc/adr/0001-test-documentation-style.md diff --git a/MANIFEST.in b/MANIFEST.in index a9001a952..9d502973f 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -19,3 +19,4 @@ prune doc/reference/toctree include doc/reference/toctree/kernels/* recursive-exclude * *.h5 recursive-exclude * *~ +prune doc/adr diff --git a/doc/adr/0001-test-documentation-style.md b/doc/adr/0001-test-documentation-style.md new file mode 100644 index 000000000..348af28cf --- /dev/null +++ b/doc/adr/0001-test-documentation-style.md @@ -0,0 +1,54 @@ +# ADR 1. Documenting Unit Tests +## Context +- [PR #721](https://github.com/NeuralEnsemble/elephant/pull/721): the new tests in `test_datasets.py` use a structured docstring (`Intent` / `Condition` / `Expected`). Elephant has a mixture of `#` comments and docstrings to describe tests, so this introduces a new style. +- Few of the `test_*` functions in `elephant/test/` have a docstring, the rest have a `#` comment block at the top of the body or nothing +- `doc/contribute.rst` says how to add a test, not how to describe it. +- Intrestingly `unittest -v` shows the first docstring line as the test description, `#` comments aren't shown anywhere, but pytest doesn't have this feature by default unless we use a plugin. + +Current style (comment): +```python +def test_download_with_cache_dir(self): + # This test is expected in the GitHub Actions environment where the + # variable is set to a local cache path. This tests that the function + # correctly uses the cache directory and returns the expected file + # path without re-downloading. +``` + +Style in PR #721 (structured docstring): +```python +def test_valid_data_version_forbidden(self): + """ + Intent: Ensure only a missing version (404) falls back to master + Condition: Version probe of `elephant-data` answers 403 + Expected: HTTPError 403 raised, nothing downloaded from master + """ +``` +Note: The Intent / Condition / Expected were made up ad hoc while writing the PR #721 tests, without knowing BDD's (Behavious Driven Development) Given / When / Then exists, which is a followed convention. + +## Options +| | Option | Pro | Con | +| --- | ----------------------------------------------- | ----------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------- | +| 1 | Keep as is, some docstring with a descriptive test name | | 1. Reviewers need to work out what a test checks from its body and/or test name
2. Really long test names| +| 2 | Structured docstring `Intent` / `Condition` / `Expected` | 1. States why, the case, and the outcome for each test
2. Missing or vague field is easy to spot in review | 1. Maybe overkill for trivial tests
2. Own field names, not a known standard | +| 3 | Structured docstring `Given` / `When` / `Then` | 1. Widely known (BDD, Gherkin) | 1. No field for *why* (Intent) the test exists
2. Could implies we use BDD tooling | + +## Decision +Accepted: Option 2 +- Make it a requirment for new tests and tests whose body a PR changes. +- Would only be needed for non-trivial tests, where Intent in not immediately clear. +- `Intent`: why the test exists (behaviour or bug it protects), recommend to start this with a verb like "Ensure", "Reproduce" etc.. +- `Condition`: the input or environment that makes this case different from the others in the class +- `Expected`: the observable outcome that the assertions check +- `Context`: (Optional) Longer explaination of the test +- `Tag`: Group unit test, e.g. `type-regression` +- Instructions to add unit-test docstrings added to `doc/contribute.rst` as well. + +## Discussion +- Should there be a context field to explain the test in detail. + - In essense should we limit the fields to one sentence. + - Decision: Not needed to limit to one sentence, also can have an optional field called "context" to have longer explanations. +- Field names: keep `Intent` / `Condition` / `Expected`, or use BDD notation with an extra `Intent` line? + - Decision: Go with Intent, Condition, Expected with an optional Context +- Any other suggestions + - Would be nice to have a tag to mention something like a regression test + - Decision: Have one more field called `tag/type` to group unit tests. From 3f4a909f6853e5fcc259e03066bf4cdaa7503946 Mon Sep 17 00:00:00 2001 From: Harris Antony Jos Date: Tue, 3 Nov 2026 09:52:43 +0100 Subject: [PATCH 8/8] [DOC] Add test docstring guidelines to contribution guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a "Describing tests" section to the contribution guide, defining a structured docstring format (Intent, Condition, Expected, optional Context/Tag) for non‑trivial tests to improve readability and maintenance --- doc/contribute.rst | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/doc/contribute.rst b/doc/contribute.rst index d3a809f34..b2b04fde0 100644 --- a/doc/contribute.rst +++ b/doc/contribute.rst @@ -221,6 +221,37 @@ something like assert_array_almost_equal(result.magnitude, expected_magnitude) ... # more checking +Describing tests +~~~~~~~~~~~~~~~~ + +New tests, and existing tests whose body your pull request changes, should +have a structured docstring if what they check is not obvious from the name +and a quick look at the body, something like: + +.. code-block:: python + + def test_valid_data_version_forbidden(self): + """ + Intent: Ensure only a missing version (404) falls back to master + Condition: Version probe of `elephant-data` answers 403 + Expected: HTTPError 403 raised, nothing downloaded from master + """ +Trivial tests can go without one. + +Fields: + +* ``Intent``: why the test exists, i.e. the behaviour or bug it protects. + Start it with a verb, e.g. "Ensure ..." or "Reproduce ...". +* ``Condition``: the input or environment that makes this case different from + the other tests in the class. +* ``Expected``: the observable outcome that the assertions check. +* ``Context`` (optional): a longer explanation of the test, e.g. background on + the bug or why the setup looks the way it does. +* ``Tag`` (optional): a label to group tests, e.g. ``type-regression``. + +Fields are not limited to one sentence; indent continuation lines to align +with the text of the first line. + Pushing the changes and creating a pull request -----------------------------------------------