Localserver for elephant-dataset

.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."

MANIFEST.in

@@ -19,3 +19,4 @@ prune doc/reference/toctree
 include doc/reference/toctree/kernels/*
 recursive-exclude * *.h5
 recursive-exclude * *~
+prune doc/adr

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<br>2. Really long test names|
+| 2   | Structured docstring `Intent` / `Condition` / `Expected` | 1. States why, the case, and the outcome for each test<br>2. Missing or vague field is easy to spot in review | 1. Maybe overkill for trivial tests<br>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<br>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.

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
 -----------------------------------------------

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"