Skip to content

basilisp test always emits a PytestAssertRewriteWarning message #1252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
ikappaki opened this issue Apr 29, 2025 · 4 comments · May be fixed by #1255
Open

basilisp test always emits a PytestAssertRewriteWarning message #1252

ikappaki opened this issue Apr 29, 2025 · 4 comments · May be fixed by #1255
Labels
component:basilisp.test Issue pertaining to basilisp.test namespace issue-type:bug Something isn't working

Comments

@ikappaki
Copy link
Contributor

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
...
  1. 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 chrisrink10 added issue-type:bug Something isn't working component:basilisp.test Issue pertaining to basilisp.test namespace labels Apr 30, 2025
@chrisrink10
Copy link
Member

I like option 1.

@chrisrink10 chrisrink10 added this to the Release v0.4.0 milestone Apr 30, 2025
@ikappaki ikappaki linked a pull request Apr 30, 2025 that will close this issue
Open
@chrisrink10
Copy link
Member

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.

@chrisrink10 chrisrink10 removed this from the Release v0.4.0 milestone May 1, 2025
@ikappaki
Copy link
Contributor Author

ikappaki commented May 2, 2025

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

@ikappaki
Copy link
Contributor Author

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

pytest-dev/pytest#13429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component:basilisp.test Issue pertaining to basilisp.test namespace issue-type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

suppress pytest rewrite assertion warning ikappaki/basilisp
2 participants
@chrisrink10 @ikappaki