diff --git a/.github/workflows/canary_ci.yml b/.github/workflows/canary_ci.yml
index b88424605..d63777e48 100644
--- a/.github/workflows/canary_ci.yml
+++ b/.github/workflows/canary_ci.yml
@@ -79,3 +79,17 @@ 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"
+ # 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."
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.
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
-----------------------------------------------
diff --git a/elephant/datasets.py b/elephant/datasets.py
index d03b1874a..f14365df9 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`).
@@ -410,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
@@ -430,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 0817a01ae..64f8a6c5a 100644
--- a/elephant/test/test_datasets.py
+++ b/elephant/test/test_datasets.py
@@ -1,15 +1,22 @@
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
+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
@@ -21,11 +28,81 @@
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"
+
+# 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):
- @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()
+ cls.addClassCleanup(cls._serve_dir.cleanup)
+ 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.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()
+ 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
+ # 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 +112,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 +122,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 +142,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 +157,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 +170,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 +195,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 +218,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 +235,69 @@ 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())
+
+ @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")
@@ -290,6 +422,69 @@ def test_download_with_cache_dir_invalid_file(self):
self.assertIn(f"'{repo_path}' does not exist in that path",
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
+ """
+ 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):