Repository: pyodide/pyodide
Pull request: Better test discovery
This improves the pytest test discovery with the following points,
 - Running `pytest` with no arguments (or `pytest .`) now correctly collects all available tests (except for emsdk ones which require the compiler setup and have to be run separately with `make -C emsdk test`)
 - Running `pytest` on packages now skips package tests that are not built. This is done at the selenium fixture level and so will also apply to new tests.
 - `packages/test_packages_common.py::test_import` now will skip the test before loading the selenium_standalone fixture which is slow. Previously it was first loading the slow fixture, and then skipping.
 - removes `make test` as I think pytest should not be used via make, as then one loses access  to all useful flags (e.g. `--pdb`, `-n auto` (for parallelism), `-s` for printing stdout, etc)

Closes https://github.com/pyodide/pyodide/issues/1979

Problem statement:
`make test` in docker always fails due broken emsdk/tests
## 🐛 Bug

Hey, is the emsdk/tests-suite broken or sort of? When I want to run the test suite locally in a docker container, it always fails.

`make test` produces this immediately on current main branch:
```
pytest src emsdk/tests packages/*/test* pyodide-build -v
============================= test session starts ==============================
platform linux -- Python 3.9.5, pytest-6.2.4, py-1.10.0, pluggy-0.13.1 -- /usr/local/bin/python
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/src/.hypothesis/examples')
rootdir: /src, configfile: setup.cfg
plugins: hypothesis-6.14.2, asyncio-0.15.1, cov-2.12.1, forked-1.3.0, httpserver-1.0.0, instafail-0.4.2, rerunfailures-10.1, xdist-2.3.0
collecting ... collected 2653 items / 5 errors / 2 skipped / 2646 selected

==================================== ERRORS ====================================
____________________ ERROR collecting emsdk/tests/common.py ____________________
/usr/local/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1030: in _gcd_import
    ???
<frozen importlib._bootstrap>:1007: in _find_and_load
    ???
<frozen importlib._bootstrap>:984: in _find_and_load_unlocked
    ???
E   ModuleNotFoundError: No module named 'tests.common'
_________________ ERROR collecting emsdk/tests/test_dyncall.py _________________
/usr/local/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1030: in _gcd_import
    ???
<frozen importlib._bootstrap>:1007: in _find_and_load
    ???
<frozen importlib._bootstrap>:984: in _find_and_load_unlocked
    ???
E   ModuleNotFoundError: No module named 'tests.test_dyncall'
_________________ ERROR collecting emsdk/tests/test_dyncall.py _________________
ImportError while importing test module '/src/emsdk/tests/test_dyncall.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/local/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
E   ModuleNotFoundError: No module named 'tests.test_dyncall'
_________________ ERROR collecting emsdk/tests/test_emulate.py _________________
/usr/local/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1030: in _gcd_import
    ???
<frozen importlib._bootstrap>:1007: in _find_and_load
    ???
<frozen importlib._bootstrap>:984: in _find_and_load_unlocked
    ???
E   ModuleNotFoundError: No module named 'tests.test_emulate'
_________________ ERROR collecting emsdk/tests/test_emulate.py _________________
ImportError while importing test module '/src/emsdk/tests/test_emulate.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/local/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
E   ModuleNotFoundError: No module named 'tests.test_emulate'
=========================== short test summary info ============================
ERROR emsdk/tests/common.py - ModuleNotFoundError: No module named 'tests.com...
ERROR emsdk/tests/test_dyncall.py - ModuleNotFoundError: No module named 'tes...
ERROR emsdk/tests/test_dyncall.py
ERROR emsdk/tests/test_emulate.py - ModuleNotFoundError: No module named 'tes...
ERROR emsdk/tests/test_emulate.py
!!!!!!!!!!!!!!!!!!! Interrupted: 5 errors during collection !!!!!!!!!!!!!!!!!!!!
========================= 2 skipped, 5 errors in 1.62s =========================
make: *** [Makefile:128: test] Error 2
```

When I only run 
```
pytest src packages/*/test* pyodide-build -v
```
it looks much better and as expected.

### To Reproduce

- Checkout current `main` branch
- `./run_docker`
- `make && make test`

Hints from issue discussion:
I'm not sure if it's related, but we also see emsdk test failures on #1677.
Should be fixed in https://github.com/pyodide/pyodide/pull/1981 which removes the `make test` command. The emdsk tests have to be currently run separately with `make -C emsdk test` since, unlike the rest of the tests, they require all the compiler setup.

Commit message: More fixes

Patch to review:
diff --git a/Makefile b/Makefile
index 8a584493fc7..7fcb5999b71 100644
--- a/Makefile
+++ b/Makefile
@@ -124,8 +124,6 @@ update_base_url: \
 	build/webworker.js
 
 
-test: all
-	pytest src emsdk/tests packages/*/test* pyodide-build -v
 
 lint: node_modules/.installed
 	# check for unused imports, the rest is done by black
diff --git a/conftest.py b/conftest.py
index a06ce7988ba..497915e2e93 100644
--- a/conftest.py
+++ b/conftest.py
@@ -1,7 +1,7 @@
 """
 Various common utilities for testing.
 """
-
+import re
 import contextlib
 import json
 import multiprocessing
@@ -61,6 +61,21 @@ def cwd_relative_nodeid(*args):
     config.cwd_relative_nodeid = cwd_relative_nodeid
 
 
+def pytest_collection_modifyitems(config, items):
+    """Called after collect is completed.
+    Parameters
+    ----------
+    config : pytest config
+    items : list of collected items
+    """
+    for item in items:
+        _maybe_skip_test(item, delayed=True)
+
+
+def _package_is_built(package_name: str) -> bool:
+    return (BUILD_PATH / f"{package_name}.data").exists()
+
+
 class JavascriptException(Exception):
     def __init__(self, msg, stack):
         self.msg = msg
@@ -463,8 +478,57 @@ def extra_checks_test_wrapper(selenium, trace_hiwire_refs, trace_pyproxies):
         assert delta_keys <= 0
 
 
+def _maybe_skip_test(item, delayed=False):
+    """If necessary skip test at the fixture level, to avoid
+
+    loading the selenium_standalone fixture which takes a long time.
+    """
+    skip_msg = None
+    # Testing a package. Skip the test if the package is not built.
+    match = re.match(
+        r".*/packages/(?P<name>[\w\-]+)/test_[\w\-]+\.py", str(item.parent.fspath)
+    )
+    if match:
+        package_name = match.group("name")
+        if not _package_is_built(package_name):
+            skip_msg = f"package '{package_name}' is not built."
+
+    # Common package import test. Skip it if the package is not built.
+    if str(item.fspath).endswith("test_packages_common.py") and item.name.startswith(
+        "test_import"
+    ):
+        match = re.match(
+            r"test_import\[(firefox|chrome|node)-(?P<name>[\w-]+)\]", item.name
+        )
+        if match:
+            package_name = match.group("name")
+            if not _package_is_built(package_name):
+                # If the test is going to be skipped remove the
+                # selenium_standalone as it takes a long time to initialize
+                skip_msg = f"package '{package_name}' is not built."
+        else:
+            raise AssertionError(
+                f"Couldn't parse package name from {item.name}. This should not happen!"
+            )
+
+    # TODO: also use this hook to skip doctests we cannot run (or run them
+    # inside the selenium wrapper)
+
+    if skip_msg is not None:
+        if delayed:
+            item.add_marker(pytest.mark.skip(reason=skip_msg))
+        else:
+            pytest.skip(skip_msg)
+
+
 @contextlib.contextmanager
 def selenium_common(request, web_server_main, load_pyodide=True):
+    """Returns an initialized selenium object.
+
+    If `_should_skip_test` indicate that the test will be skipped,
+    return None, as initializing Pyodide for selenium is expensive
+    """
+
     server_hostname, server_port, server_log = web_server_main
     if request.param == "firefox":
         cls = FirefoxWrapper
@@ -488,6 +552,9 @@ def selenium_common(request, web_server_main, load_pyodide=True):
 
 @pytest.fixture(params=["firefox", "chrome", "node"], scope="function")
 def selenium_standalone(request, web_server_main):
+    # Avoid loading the fixture if the test is going to be skipped
+    _maybe_skip_test(request.node)
+
     with selenium_common(request, web_server_main) as selenium:
         with set_webdriver_script_timeout(
             selenium, script_timeout=parse_driver_timeout(request)
@@ -500,6 +567,9 @@ def selenium_standalone(request, web_server_main):
 
 @contextlib.contextmanager
 def selenium_standalone_noload_common(request, web_server_main):
+    # Avoid loading the fixture if the test is going to be skipped
+    _maybe_skip_test(request.node)
+
     with selenium_common(request, web_server_main, load_pyodide=False) as selenium:
         with set_webdriver_script_timeout(
             selenium, script_timeout=parse_driver_timeout(request)
@@ -512,6 +582,8 @@ def selenium_standalone_noload_common(request, web_server_main):
 
 @pytest.fixture(params=["firefox", "chrome"], scope="function")
 def selenium_webworker_standalone(request, web_server_main):
+    # Avoid loading the fixture if the test is going to be skipped
+    _maybe_skip_test(request.node)
     with selenium_standalone_noload_common(request, web_server_main) as selenium:
         yield selenium
 
@@ -520,6 +592,8 @@ def selenium_webworker_standalone(request, web_server_main):
 def selenium_standalone_noload(request, web_server_main):
     """Only difference between this and selenium_webworker_standalone is that
     this also tests on node."""
+    # Avoid loading the fixture if the test is going to be skipped
+    _maybe_skip_test(request.node)
     with selenium_standalone_noload_common(request, web_server_main) as selenium:
         yield selenium
 
diff --git a/docs/development/testing.md b/docs/development/testing.md
index 1b3d9c69d02..16514835780 100644
--- a/docs/development/testing.md
+++ b/docs/development/testing.md
@@ -21,7 +21,7 @@ and check that they are in your `PATH`.
 To run the pytest suite of tests, type on the command line:
 
 ```bash
-pytest src/ pyodide-build/ packages/*/test_*
+pytest
 ```
 
 There are 3 test locations that are collected by pytest,
@@ -31,6 +31,12 @@ There are 3 test locations that are collected by pytest,
   (do not require selenium to run)
 - `packages/*/test_*`: package specific tests.
 
+Additionally you can run emsdk specific tests with,
+
+```
+make -C emsdk test
+```
+
 ### Running the JavaScript test suite
 
 To run tests on the JavaScript Pyodide package using Mocha, run the following commands,
diff --git a/packages/test_packages_common.py b/packages/test_packages_common.py
index de775baf9c0..95a8058b14b 100644
--- a/packages/test_packages_common.py
+++ b/packages/test_packages_common.py
@@ -72,7 +72,12 @@ def test_parse_package(name):
 @pytest.mark.driver_timeout(40)
 @pytest.mark.parametrize("name", registered_packages())
 def test_import(name, selenium_standalone):
-    # check that we can parse the meta.yaml
+    if name not in built_packages():
+        raise AssertionError(
+            "Implementation error. Test for an unbuilt package "
+            "should have been skipped in selenium_standalone fixture"
+        )
+
     meta = parse_package_config(PKG_DIR / name / "meta.yaml")
 
     if name in UNSUPPORTED_PACKAGES[selenium_standalone.browser]:
@@ -82,9 +87,6 @@ def test_import(name, selenium_standalone):
             )
         )
 
-    if name not in built_packages():
-        pytest.skip(f"{name} was skipped due to PYODIDE_PACKAGES")
-
     selenium_standalone.run("import glob, os")
 
     baseline_pyc = selenium_standalone.run(
diff --git a/pyodide-build/pyodide_build/testing.py b/pyodide-build/pyodide_build/testing.py
index 0adad236299..4762eaf130e 100644
--- a/pyodide-build/pyodide_build/testing.py
+++ b/pyodide-build/pyodide_build/testing.py
@@ -132,7 +132,7 @@ def set_webdriver_script_timeout(selenium, script_timeout: Optional[Union[int, f
     Parameters
     ----------
     selenum : SeleniumWrapper
-       a SeleniumWrapper wrapper instance
+       a SeleniumWrapper wrapper instance, or None
     script_timeout : int | float
        value of the timeout in seconds
     """
diff --git a/setup.cfg b/setup.cfg
index a07eea1af95..af343c18cea 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,10 +1,21 @@
 [tool:pytest]
-norecursedirs = build cpython emsdk/
-addopts = --doctest-modules
+norecursedirs =
+    build
+    cpython
+    emsdk
+    .artifacts
+addopts =
+    --doctest-modules
+    --ignore="packages/matplotlib/src"
 markers =
     skip_refcount_check: Dont run refcount checks
     skip_pyproxy_check: Dont run pyproxy allocation checks
     driver_timeout: Set script timeout in WebDriver
+testpaths =
+    src
+    pyodide-build
+    packages
+
 
 [bumpversion]
 current_version = 0.18.0


Code review comments:
diff_hunk: @@ -463,8 +478,57 @@ def extra_checks_test_wrapper(selenium, trace_hiwire_refs, trace_pyproxies):
         assert delta_keys <= 0
 
 
+def _maybe_skip_test(item, delayed=False):
+    """If necessary skip test at the fixture level, to avoid
+
+    loading the selenium_standalone fixture which takes a long time.
+    """
+    skip_msg = None
+    # Testing a package. Skip the test if the package is not built.
+    match = re.match(
+        r".*/packages/(?P<name>[\w\-]+)/test_[\w\-]+\.py", str(item.parent.fspath)
+    )
+    if match:
+        package_name = match.group("name")
+        if not _package_is_built(package_name):
+            skip_msg = f"package '{package_name}' is not built."
+
+    # Common package import test. Skip it if the package is not built.
+    if str(item.fspath).endswith("test_packages_common.py") and item.name.startswith(
path: conftest.py
text: @user1:
Is possibly a test like this missing?
```suggestion
    if not skip_msg and str(item.fspath).endswith("test_packages_common.py") and item.name.startswith(
```

@author:
Indeed it wouldn't hurt to check that.