`basilisp test` always emits a `PytestAssertRewriteWarning` message

[ikappaki]

Hi,

basilisp test always emits a PytestAssertRewriteWarning when run in a Basilisp project:

PytestAssertRewriteWarning: Module already imported so cannot be rewritten: basilisp

To reproduce:

1. Create a new project with Poetry and add basilisp and pytest:

PS > poetry new basproj
Created package basproj in basproj
PS > cd basproj
PS basproj> poetry add basilisp pytest
Creating virtualenv basproj in basproj\.venv
Using version ^0.3.8 for basilisp
Using version ^8.3.5 for pytest
...

2. run basilisp test, the above warning is emitted:

PS basproj> poetry run basilisp test
================================================= test session starts 
platform win32 -- Python 3.11.4, pytest-8.3.5, pluggy-1.5.0
rootdir: basproj
configfile: pyproject.toml
plugins: basilisp-0.3.8
collected 0 items

================================================== warnings summary 
.venv\Lib\site-packages\_pytest\config\__init__.py:1277
  basproj\.venv\Lib\site-packages\_pytest\config\__init__.py:1277: PytestAssertRewriteWarning: Module already imported so cannot be rewritten: basilisp
    self._mark_plugins_for_rewrite(hook)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================= 1 warning in 0.01s 

The warning occurs because basilisp declares a pytest plugin in its pyproject.toml:

[tool.poetry.plugins.pytest11]
basilisp_test_runner = "basilisp.contrib.pytest.testrunner"

which results in the following entry point in the installed distribution:

# .venv/Lib/site-packages/basilisp-0.3.8.dist-info/entry_points.txt
[console_scripts]
basilisp=basilisp.cli:invoke_cli
basilisp-run=basilisp.cli:run_script

[pytest11]
basilisp_test_runner=basilisp.contrib.pytest.testrunner

As per assertion rewriting doc, any module registered as a [pytest11] plugin is subject to assertion rewriting. However, in cli.py, basilisp is imported before pytest.main() is called, so pytest can't reimport the module for the purpose of rewriting its assertions, hence the warning.

Options I can think of:

1. Silence the warning:
   Since assertion rewriting is likely unnecessary for the Basilisp test runner plugin, this warning can probably be safely ignore. We can explicitly suppress this warning before calling pytest.main().

2. Unload the basilisp module before calling pytest:
   Using del sys.modules["basilisp"] would technically allow pytest to reimport the module and rewrite it, but this is risky and could lead to inconsistencies, especially if anything from basilisp has already been used.

3. Extract the test runner plugin into a separate package:
   This avoids the warning entirely but comes at the cost of creating and maintaining a new separate basilisp-testrunner package.

I'm leaning toward option 1, and plan to raise a PR for it. If you have a better idea or prefer a different solution, please feel free to suggest.

Thanks

[chrisrink10]

I like option 1.

[chrisrink10]

I looked a bit more into the options here and I'm just not really sure any of the options presented are right or that this issue can be fixed by Basilisp in a good way. I found a related issue (pytest-dev/pytest#5473) in Pytest about this exact problem, which makes me think maybe Pytest needs a better functionality for suppressing this warning in such cases.

If we go with option (1) (as you presented in PR #1255), then we will suppress that warning for presumably every plugin which might not be the desired behavior if users end up using other Pytest plugins. The warning text seems to disagree with their documentation which suggests only the module named in the plugin needs to be rewritten, since it is complaining about basilisp module specifically.

I think option (3) may work, but that would introduce additional complexity in requiring the testrunner to be a separate module. Since a significant portion of the Basilisp test suite depends on basilisp.contrib.pytest.testrunner and that testrunner depends on Basilisp, I'm not sure they can realistically be split up since they are circularly dependent.

For now, I think if the warning is an issue it seems you can suppress it by invoking Pytest as either pytest ... or python -m pytest. Both should work relatively equivalently to basilisp test since the testrunner plugin bootstraps Basilisp in it's pytest_configure hook.

[ikappaki]

If we go with option (1) (as you presented in PR #1255), then we will suppress that warning for presumably every plugin which might not be the desired behavior if users end up using other Pytest plugins. The warning text seems to disagree with their documentation which suggests only the module named in the plugin needs to be rewritten, since it is complaining about basilisp module specifically.

Actually, Pytest does mention basilisp and thus it provides the potential to filter the warning only for Basilisp:

Module already imported so cannot be rewritten: basilisp.

Unfortunately, the : in the message interferes with the -W warning filter syntax, which also uses : as a field separator:

-W ignore:Module already imported so cannot be rewritten: basilisp:pytest.PytestAssertRewriteWarning

Since -W does not support escaping : or using regex, I opted for a more general filter without mentioning ...: basilisp in the PR, which will suppress the warning for any plugin, not just Basilsp:

-W ignore:Module already imported so cannot be rewritten:pytest.PytestAssertRewriteWarning

unfortunately there does not appear to be a way to a escape a : in the filter message, and it has to be a literal string, regex is not supported.

Trying to add an ignore filter with warning.filterwarnings

import warnings
warnings.filterwarnings("ignore", message="Module already imported so cannot be rewritten: basilisp", category=pytest.PytestAssertRewriteWarning)

doesn't work either because the function that emits the warning temporarily disables all filters:
https://github.com/pytest-dev/pytest/blob/20c51f70c99ecaa4745d622a69bbe93fbf59ad16/src/_pytest/config/__init__.py#L1541-L1574

I plan to suggest to the Pytest team that they consider removing the : from such warnings to allow more precise filtering at runtime.

As another manual workaround, regex-based filtering in the user's pyproject.toml can help in the meantime to suppress the error, although is not ideal (notice the : has been replaced with the . regex char):

pyproject.toml

[tool.pytest.ini_options]
filterwarnings = [
    "ignore:Module already imported so cannot be rewritten. basilisp:pytest.PytestAssertRewriteWarning",
]

I think option (3) may work, but that would introduce additional complexity in requiring the testrunner to be a separate module. Since a significant portion of the Basilisp test suite depends on basilisp.contrib.pytest.testrunner and that testrunner depends on Basilisp, I'm not sure they can realistically be split up since they are circularly dependent.

For now, I think if the warning is an issue it seems you can suppress it by invoking Pytest as either pytest ... or python -m pytest. Both should work relatively equivalently to basilisp test since the testrunner plugin bootstraps Basilisp in it's pytest_configure hook.

It's more of an annoyance than a show-stopper, so I don't think there is a need to call pytest directly. We can live with it, but IMHO it should be addressed eventually.

thanks