Improve test coverage on load-liblsl and on datasets (#176)

README.md

@@ -4,6 +4,7 @@
 [![tests](https://github.com/mne-tools/mne-lsl/actions/workflows/pytest.yml/badge.svg?branch=main)](https://github.com/mne-tools/mne-lsl/actions/workflows/pytest.yml)
 [![doc](https://github.com/mne-tools/mne-lsl/actions/workflows/doc.yml/badge.svg?branch=main)](https://github.com/mne-tools/mne-lsl/actions/workflows/doc.yml)
 [![PyPI version](https://badge.fury.io/py/mne-lsl.svg)](https://badge.fury.io/py/mne-lsl)
+[![Downloads](https://static.pepy.tech/badge/mne-lsl)](https://pepy.tech/project/mne-lsl)
 [![Conda Version](https://img.shields.io/conda/vn/conda-forge/mne-lsl.svg)](https://anaconda.org/conda-forge/mne-lsl)
 [![Conda Downloads](https://img.shields.io/conda/dn/conda-forge/mne-lsl.svg)](https://anaconda.org/conda-forge/mne-lsl)
 [![Conda Platforms](https://img.shields.io/conda/pn/conda-forge/mne-lsl.svg)](https://anaconda.org/conda-forge/mne-lsl)

doc/changes/authors.inc

@@ -1,3 +1,4 @@
 .. _Arnaud Desvachez: https://github.com/dnastars
+.. _Eric Larson: https://github.com/larsoner
 .. _Kyuhwa Lee: https://github.com/dbdq
 .. _Mathieu Scheltienne: https://github.com/mscheltienne

doc/changes/latest.rst

@@ -27,14 +27,10 @@ Enhancements
 Bugs
 ----
 
-- xxx
+- Fix flaky tests on CIs (:pr:`168` by `Eric Larson`_ and `Mathieu Scheltienne`_)
+- Fix re-download of existing ``liblsl`` on macOS and test ``liblsl`` fetching (:pr:`176` by `Mathieu Scheltienne`_)
 
 API and behavior changes
 ------------------------
 
 - xxx
-
-Authors
--------
-
-* xxx

doc/resources/install.rst

@@ -8,8 +8,8 @@ Default install
 
 ``MNE-LSL`` requires Python version ``3.9`` or higher and is available on
 `PyPI <project pypi_>`_. It requires ``liblsl`` which will be either fetch from the path
-in the environment variable ``MNE_LSL_LIB``, or from the system directories or
-downloaded from the release page on compatible platforms.
+in the environment variable ``MNE_LSL_LIB`` (or ``PYLSL_LIB``), or from the system
+directories or downloaded from the release page on compatible platforms.
 
 .. tab-set::
 
@@ -37,7 +37,7 @@ Different liblsl version
 If you prefer to use a different version of `liblsl <lsl lib c++_>`_ than the
 automatically downloaded one, or if your platform is not supported, you can build
 `liblsl <lsl lib c++_>`_ from source and provide the path to the library in an
-environment variable ``MNE_LSL_LIB``.
+environment variable ``MNE_LSL_LIB`` (or ``PYLSL_LIB``).
 
 liblsl and LabRecorder dependencies
 -----------------------------------

doc/resources/pylsl.rst

@@ -69,8 +69,8 @@ present in the :py:attr:`mne_lsl.lsl.StreamInfo.desc` property.
 Improve arguments
 ~~~~~~~~~~~~~~~~~
 
-The arguments of a `~mne_lsl.lsl.StreamInfo`, `~mne_lsl.lsl.StreamInlet`,
-`~mne_lsl.lsl.StreamOutlet` support a wider variety of types. For instance:
+The arguments of a :class:`~mne_lsl.lsl.StreamInfo`, :class:`~mne_lsl.lsl.StreamInlet`,
+:class:`~mne_lsl.lsl.StreamOutlet` support a wider variety of types. For instance:
 
 * ``dtype``, which correspond to the ``channel_format`` in `pylsl <lsl python_>`_, can
   be provided as a string or as a supported :class:`numpy.dtype`, e.g. ``np.int8``.

mne_lsl/conftest.py

@@ -68,10 +68,13 @@ def pytest_sessionfinish(session, exitstatus) -> None:
 
 
 def _closer():
-    # We cannot rely on just "del inlet" / "del outlet" because Python's garbage
-    # collector can run whenever it feels like it, and AFAIK the garbage collection
-    # order is not guaranteed. So let's explicitly __del__ ourselves, knowing that
-    # our __del__s are smart enough to be no-ops if called more than once.
+    """Delete inlet, outlet, and player vars if present.
+
+    We cannot rely on just "del inlet" / "del outlet" because Python's garbage collector
+    can run whenever it feels like it, and AFAIK the garbage collection order is not
+    guaranteed. So let's explicitly __del__ ourselves, knowing that our __del__s are
+    smart enough to be no-ops if called more than once.
+    """
     loc = inspect.currentframe().f_back.f_locals
     for name in ("inlet", "outlet"):
         if name in loc:
@@ -108,7 +111,7 @@ def raw(fname) -> Raw:
 @fixture(scope="function")
 def mock_lsl_stream(fname, request):
     """Create a mock LSL stream for testing."""
-    # nest PlayerLSL import so temp config gets written first
+    # nest the PlayerLSL import to first write the temporary LSL configuration file
     from mne_lsl.player import PlayerLSL  # noqa: E402
 
     name = f"P_{request.node.name}"

mne_lsl/datasets/sample.py

@@ -13,6 +13,8 @@
 if TYPE_CHECKING:
     from typing import Optional, Union
 
+_REGISTRY = files("mne_lsl.datasets") / "sample-registry.txt"
+
 
 def _make_registry(
     folder: Union[str, Path], output: Optional[Union[str, Path]] = None
@@ -27,9 +29,7 @@ def _make_registry(
         Path to the output registry file.
     """
     folder = ensure_path(folder, must_exist=True)
-    output = (
-        files("mne_lsl.datasets") / "sample-registry.txt" if output is None else output
-    )
+    output = _REGISTRY if output is None else output
     pooch.make_registry(folder, output=output, recursive=True)
 
 

mne_lsl/datasets/testing.py

@@ -13,6 +13,8 @@
 if TYPE_CHECKING:
     from typing import Optional, Union
 
+_REGISTRY = files("mne_lsl.datasets") / "testing-registry.txt"
+
 
 def _make_registry(
     folder: Union[str, Path], output: Optional[Union[str, Path]] = None
@@ -27,9 +29,7 @@ def _make_registry(
         Path to the output registry file.
     """
     folder = ensure_path(folder, must_exist=True)
-    output = (
-        files("mne_lsl.datasets") / "testing-registry.txt" if output is None else output
-    )
+    output = _REGISTRY if output is None else output
     pooch.make_registry(folder, output=output, recursive=True)
 
 

mne_lsl/datasets/tests/test_datasets.py

@@ -0,0 +1,23 @@
+import pytest
+
+from mne_lsl.datasets import sample, testing
+from mne_lsl.datasets.sample import _REGISTRY as _REGISTRY_SAMPLE
+from mne_lsl.datasets.testing import _REGISTRY as _REGISTRY_TESTING
+from mne_lsl.utils._tests import sha256sum
+
+
+@pytest.mark.parametrize("dataset", [sample, testing])
+def test_data_path(dataset):
+    """Test download if the testing dataset."""
+    path = dataset.data_path()
+    assert path.exists()
+
+
+@pytest.mark.parametrize(
+    "dataset, registry", [(sample, _REGISTRY_SAMPLE), (testing, _REGISTRY_TESTING)]
+)
+def test_make_registry(tmp_path, dataset, registry):
+    """Test the registrytmp_path making."""
+    dataset._make_registry(dataset.data_path(), output=tmp_path / "registry.txt")
+    assert (tmp_path / "registry.txt").exists()
+    assert sha256sum(tmp_path / "registry.txt") == sha256sum(registry)

mne_lsl/lsl/functions.py

@@ -106,7 +106,6 @@ def resolve_streams(
         streams = [_BaseStreamInfo(buffer[k]) for k in range(num_found)]
         streams = list(set(streams))  # remove duplicates
         return streams
-
     minimum = ensure_int(minimum, "minimum")
     if minimum <= 0:
         raise ValueError(
@@ -127,7 +126,6 @@ def resolve_streams(
         check_type(prop, (str,), name)
         # rename properties for lsl compatibility
         name = "type" if name == "stype" else name
-
         num_found = lib.lsl_resolve_byprop(
             byref(buffer),
             1024,
@@ -137,7 +135,7 @@ def resolve_streams(
             c_double(timeout),
         )
         new_streams = [_BaseStreamInfo(buffer[k]) for k in range(num_found)]
-        # now delete the ones that dn't have all the correct properties
+        # now delete the ones that don't have all the correct properties
         stream2delete = list()
         for k, stream in enumerate(new_streams):
             for prop, name in properties:

mne_lsl/lsl/load_liblsl.py

@@ -41,9 +41,9 @@
 }
 # generic error message
 _ERROR_MSG = (
-    "Please visit liblsl library github page "
-    + "(https://github.com/sccn/liblsl) and install a release in the system "
-    + "directories or provide its path in the environment variable MNE_LSL_LIB."
+    "Please visit liblsl library github page (https://github.com/sccn/liblsl) and "
+    "install a release in the system directories or provide its path in the "
+    "environment variable MNE_LSL_LIB or PYLSL_LIB."
 )
 
 
@@ -68,7 +68,11 @@ def _find_liblsl() -> Optional[CDLL]:
     lib : CDLL | None
         Loaded binary LSL library. None if not found.
     """
-    for libpath in (os.environ.get("MNE_LSL_LIB", None), find_library("lsl")):
+    for libpath in (
+        os.environ.get("MNE_LSL_LIB", None),
+        os.environ.get("PYLSL_LIB", None),
+        find_library("lsl"),
+    ):
         if libpath is None:
             continue
 
@@ -114,9 +118,14 @@ def _find_liblsl() -> Optional[CDLL]:
     return lib
 
 
-def _fetch_liblsl() -> Optional[CDLL]:
+def _fetch_liblsl(folder: Path = files("mne_lsl.lsl") / "lib") -> Optional[CDLL]:
     """Fetch liblsl on the release page.
 
+    Parameters
+    ----------
+    folder : Path
+        Folder where the fetched liblsl is stored.
+
     Returns
     -------
     lib : CDLL | None
@@ -206,7 +215,7 @@ def _fetch_liblsl() -> Optional[CDLL]:
         )
 
     asset = assets[0]
-    folder = files("mne_lsl.lsl") / "lib"
+    logger.debug("Fetching liblsl into '%s'.", folder)
     try:
         os.makedirs(folder, exist_ok=True)
     except Exception as error:
@@ -215,7 +224,13 @@ def _fetch_liblsl() -> Optional[CDLL]:
             "MNE-LSL could not create the directory 'lib' in which to download liblsl "
             "for your platform. " + _ERROR_MSG
         )
-    libpath = (folder / asset["name"]).with_suffix(_PLATFORM_SUFFIXES[_PLATFORM])
+    if _PLATFORM == "darwin":
+        libpath = (
+            folder
+            / f"{asset['name'].split('.tar.bz2')[0]}{_PLATFORM_SUFFIXES['darwin']}"
+        )
+    else:
+        libpath = (folder / asset["name"]).with_suffix(_PLATFORM_SUFFIXES[_PLATFORM])
     if libpath.exists():
         _, version = _attempt_load_liblsl(libpath)
         if version is None:
@@ -265,9 +280,9 @@ def _pooch_processor_liblsl(fname: str, action: str, pooch: Pooch) -> str:
     fname : str
         The full path to the file in the local data storage.
     """
-    folder = files("mne_lsl.lsl") / "lib"
     fname = Path(fname)
-    uncompressed = folder / f"{fname.name}.archive"
+    uncompressed = fname.with_suffix(".archive")
+    logger.debug("Processing %s with pooch.", fname)
 
     if _PLATFORM == "linux" and fname.suffix == ".deb":
         os.makedirs(uncompressed, exist_ok=True)
@@ -307,7 +322,8 @@ def _pooch_processor_liblsl(fname: str, action: str, pooch: Pooch) -> str:
             if file.is_symlink() or file.parent.name != "lib":
                 continue
             break
-        target = (folder / fname.name).with_suffix(_PLATFORM_SUFFIXES["linux"])
+        target = fname.with_suffix(_PLATFORM_SUFFIXES["linux"])
+        logger.debug("Moving '%s' to '%s'.", file, target)
         move(file, target)
 
     elif _PLATFORM == "linux":
@@ -321,8 +337,10 @@ def _pooch_processor_liblsl(fname: str, action: str, pooch: Pooch) -> str:
                 continue
             break
         target = (
-            folder / f"{fname.name.split('.tar.bz2')[0]}{_PLATFORM_SUFFIXES['darwin']}"
+            fname.parent
+            / f"{fname.name.split('.tar.bz2')[0]}{_PLATFORM_SUFFIXES['darwin']}"
         )
+        logger.debug("Moving '%s' to '%s'.", file, target)
         move(file, target)
 
     elif _PLATFORM == "windows":
@@ -335,7 +353,8 @@ def _pooch_processor_liblsl(fname: str, action: str, pooch: Pooch) -> str:
             ):
                 continue
             break
-        target = (folder / fname.name).with_suffix(_PLATFORM_SUFFIXES["windows"])
+        target = fname.with_suffix(_PLATFORM_SUFFIXES["windows"])
+        logger.debug("Moving '%s' to '%s'.", file, target)
         move(file, target)
 
     # clean-up

mne_lsl/lsl/stream_info.py

@@ -256,7 +256,7 @@ def session_id(self) -> str:
 
     @property
     def uid(self) -> str:
-        """Unique ID of the `~mne_lsl.lsl.StreamOutlet` instance.
+        """Unique ID of the :class:`~mne_lsl.lsl.StreamOutlet` instance.
 
         This ID is guaranteed to be different across multiple instantiations of the same
         :class:`~mne_lsl.lsl.StreamOutlet`, e.g. after a re-start.
@@ -283,7 +283,7 @@ def as_xml(self) -> str:
 
         This yields an XML document (in string form) whose top-level element is
         ``<info>``. The info element contains one element for each field of the
-        `~mne_lsl.lsl.StreamInfo` class, including:
+        :class:`~mne_lsl.lsl.StreamInfo` class, including:
 
         * the core elements ``name``, ``type`` (eq. ``stype``), ``channel_count``
           (eq. ``n_channels``), ``nominal_srate`` (eq. ``sfreq``), ``channel_format``
@@ -661,7 +661,7 @@ def set_channel_types(self, ch_types: Union[str, list[str]]) -> None:
         ----------
         ch_types : list of str | str
             List of channel types, matching the number of total channels.
-            If a single `str` is provided, the type is applied to all channels.
+            If a single :class:`str` is provided, the type is applied to all channels.
         """
         ch_types = (
             [ch_types] * self.n_channels if isinstance(ch_types, str) else ch_types
@@ -686,7 +686,7 @@ def set_channel_units(
 
         Notes
         -----
-        Some channel types doch_units not have a unit. The `str` ``none`` or the
+        Some channel types doch_units not have a unit. The :class:`str` ``none`` or the
         :class:`int` 0 should be used to denote this channel unit, corresponding to
         ``FIFF_UNITM_NONE`` in MNE-Python.
         """

mne_lsl/lsl/stream_inlet.py

@@ -232,7 +232,7 @@ def time_correction(self, timeout: Optional[float] = None) -> float:
         time_correction : float
             Current estimate of the time correction. This number needs to be added to a
             timestamp that was remotely generated via ``local_clock()`` to map it into
-            the local clock domain of the client machine.
+            the :func:`~mne_lsl.lsl.local_clock` domain of the client machine.
         """
         timeout = _check_timeout(timeout)
         errcode = c_int()

mne_lsl/lsl/tests/test_load_liblsl.py

@@ -1,7 +1,16 @@
+import platform
+from pathlib import Path
+
 import pytest
 import requests
 
-from mne_lsl.lsl.load_liblsl import _PLATFORM, _PLATFORM_SUFFIXES, _SUPPORTED_DISTRO
+from mne_lsl import __version__
+from mne_lsl.lsl.load_liblsl import (
+    _PLATFORM,
+    _PLATFORM_SUFFIXES,
+    _SUPPORTED_DISTRO,
+    _fetch_liblsl,
+)
 
 
 def test_os_detection():
@@ -15,7 +24,11 @@ def test_os_detection():
 @pytest.mark.xfail(raises=KeyError, reason="403 Forbidden Error on GitHub API request.")
 def test_distro_support():
     """Test that the variables are in sync with the latest liblsl release."""
-    response = requests.get("https://api.github.com/repos/sccn/liblsl/releases/latest")
+    response = requests.get(
+        "https://api.github.com/repos/sccn/liblsl/releases/latest",
+        timeout=15,
+        headers={"user-agent": f"mne-lsl/{__version__}"},
+    )
     assets = [
         elt["name"]
         for elt in response.json()["assets"]
@@ -28,3 +41,31 @@ def test_distro_support():
     assert "jammy" in assets[2]  # 22.04 LTS
     # confirm that it matches _SUPPORTED_DISTRO
     assert _SUPPORTED_DISTRO["ubuntu"] == ("18.04", "20.04", "22.04")
+
+
+@pytest.mark.skipif(
+    platform.system().lower().strip() == "windows",
+    reason="PermissionError: [WinError 5] Access is denied.",
+)
+@pytest.mark.xfail(reason="403 Forbidden Error on GitHub API request.")
+def test_fetch_liblsl(tmp_path):
+    """Test on-the-fly fetch of liblsl."""
+    lib = _fetch_liblsl(tmp_path)
+    assert lib is not None
+    # don't re-download if it's already present
+    lib = _fetch_liblsl(tmp_path)
+    assert lib is not None
+    # delete the file and try again
+    fname = Path(lib._name)
+    if fname.exists():
+        fname.unlink()
+    lib = _fetch_liblsl(tmp_path)
+    assert lib is not None
+    assert lib._name == str(fname)
+    # replace the file with an invalid one and try again
+    if fname.exists():
+        fname.unlink()
+        with open(fname, "w") as file:
+            file.write("101")
+    lib = _fetch_liblsl(tmp_path)
+    assert lib is not None