Skip to content
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.

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

Can't run tests in a new Basilisp project #1069

Open
ikappaki opened this issue Sep 21, 2024 · 8 comments
Open

Can't run tests in a new Basilisp project #1069

ikappaki opened this issue Sep 21, 2024 · 8 comments
Labels
component:testing Issue pertaining to tests/testing issue-type:bug Something isn't working

Comments

@ikappaki
Copy link
Contributor

ikappaki commented Sep 21, 2024

Hi,

it's not possible to execute Basilisp tests in a new project due to the following error

ERROR tests/issuetests/issue_test.lpy - ModuleNotFoundError: No module named 'tests'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

To reproduce with poetry

  1. Create a new issuetests project, add basilisp and pytest deps, and install project
PS D:\bas> poetry new --src issuetests
...
PS D:\bas> cd .\issuetests\
...
PS D:\bas\issuetests> poetry add basilisp
Creating virtualenv issuetests in D:\bas\issuetests\.venv
Using version ^0.2.3 for basilisp
...
PS D:\bas\issuetests> poetry add pytest --dev
The --dev option is deprecated, use the `--group dev` notation instead.
Using version ^8.3.3 for pytest
...
PS D:\bas\issuetests> poetry install
...
Installing the current project: issuetests (0.1.0)
  1. Create a test file with the following content
    ./tests/issuetests/issue_test.lpy
(ns tests.issuetests.issue-test
  (:require [basilisp.test :refer [deftest are is testing]]))

(deftest xyz []
  (is (= 5 5)))
  1. Run pytest, you will encounter the "No module named 'tests' error"
PS D:\bas\issuetests> poetry run pytest
================================================= test session starts =================================================
platform win32 -- Python 3.11.4, pytest-8.3.3, pluggy-1.5.0
rootdir: D:\bas\issuetests
configfile: pyproject.toml
plugins: basilisp-0.2.3
collected 0 items / 1 error

======================================================= ERRORS ========================================================
__________________________________ ERROR collecting tests/issuetests/issue_test.lpy ___________________________________
.venv\Lib\site-packages\basilisp\contrib\pytest\testrunner.py:234: in collect
    module = self._import_module()
.venv\Lib\site-packages\basilisp\contrib\pytest\testrunner.py:222: in _import_module
    module = importlib.import_module(modname)
C:\local\Python311\Lib\importlib\__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1204: in _gcd_import
    ???
<frozen importlib._bootstrap>:1176: in _find_and_load
    ???
<frozen importlib._bootstrap>:1126: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:241: in _call_with_frames_removed
    ???
<frozen importlib._bootstrap>:1204: in _gcd_import
    ???
<frozen importlib._bootstrap>:1176: in _find_and_load
    ???
<frozen importlib._bootstrap>:1126: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:241: in _call_with_frames_removed
    ???
<frozen importlib._bootstrap>:1204: in _gcd_import
    ???
<frozen importlib._bootstrap>:1176: in _find_and_load
    ???
<frozen importlib._bootstrap>:1140: in _find_and_load_unlocked
    ???
E   ModuleNotFoundError: No module named 'tests'
=============================================== short test summary info ===============================================
ERROR tests/issuetests/issue_test.lpy - ModuleNotFoundError: No module named 'tests'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================== 1 error in 0.10s ===================================================

The same error occurs when running basilisp test.

A workaround I found to bypass this issue is to create an empty conftest.py file
./tests/conftest.py (Empty)

though this is not mentioned in the official documentation as a requirement https://basilisp.readthedocs.io/en/v0.1.1/gettingstarted.html#project-structure

PS D:\bas\issuetests> poetry run pytest
================================================= test session starts =================================================
platform win32 -- Python 3.11.4, pytest-8.3.3, pluggy-1.5.0
rootdir: D:\bas\issuetests
configfile: pyproject.toml
plugins: basilisp-0.2.3
collected 1 item

tests\issuetests\issue_test.lpy .                                                                                [100%]

================================================== 1 passed in 0.04s ==================================================

Running basilisp test gives an extra warning, but I this is unrelated to this issue

PS D:\bas\issuetests> poetry run basilisp test
================================================= test session starts =================================================
platform win32 -- Python 3.11.4, pytest-8.3.3, pluggy-1.5.0
rootdir: D:\bas\issuetests
configfile: pyproject.toml
plugins: basilisp-0.2.3
collected 1 item

tests\issuetests\issue_test.lpy .                                                                                [100%]

================================================== warnings summary ===================================================
.venv\Lib\site-packages\_pytest\config\__init__.py:1277
  D:\bas\issuetests\.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 passed, 1 warning in 0.02s =============================================

Thanks

@chrisrink10
Copy link
Member

This is an interesting one. I haven't really run tests outside of this repository yet, so not surprising I haven't run into this issue.

Aside from the conftest.py trick, I imagine you could probably just throw an __init__.py in tests/ and it might work as well? If not, we may need to do the same thing we did in #1036 with the basilisp.test CLI entrypoint and add an empty entry to the beginning of sys.path.

I think eventually whatever project tooling I end up building would do something similar to Leiningen or clj and can set the paths for tests and sources correctly.

@chrisrink10 chrisrink10 added issue-type:bug Something isn't working component:testing Issue pertaining to tests/testing labels Sep 21, 2024
@ikappaki
Copy link
Contributor Author

Aside from the conftest.py trick, I imagine you could probably just throw an __init__.py in tests/ and it might work as well? If not, we may need to do the same thing we did in #1036 with the basilisp.test CLI entrypoint and add an empty entry to the beginning of sys.path.

I think the problem is with the basilisp testrunner. sys.path already picks up the empty entry, and adding an additional __init__.py doesn't help:

> basilisp run -c "(println sys/path)"
#py ["" ... "D:\\bas\\issuetests\\src"]

I've created a minimal repo at https://github.com/ikappaki/basilisp-issue-1069 to demonstrate in practice, with the following branches

  • main: the minimal reproducible test case. A pytest fails with the above error.
  • issue/lpy-works-with-empty: the workaround where you add an empty tests/conftest.py and it magically works.
  • issue/py-test-works: replaced src/issuetests/issue_test.lpy with a tests/issuetests/issue_py_test.py, and the python test runs fine.
  • checkout issue/still-fails-with-__init__.py: added the additional tests/issuetests/__init__.py file (the tests/__init__.py was alredy present), the lpy test still fails.

I think eventually whatever project tooling I end up building would do something similar to Leiningen or clj and can set the paths for tests and sources correctly.

That would be a great addition. However, I believe the issue here is related to the test runner: .py tests work fine in this setup, but .lpy tests don’t. There's also this unexplained workaround where you need tests/conftest for the lpy tests to function properly.

Thanks

@chrisrink10
Copy link
Member

I think the problem is with the basilisp testrunner. sys.path already picks up the empty entry, and adding an additional init.py doesn't help:

It's harder to test this, but I didn't add the path fixes to basilisp test (oops!) so it could still be path related. At least as a short term fix.

@ikappaki
Copy link
Contributor Author

I think the problem is with the basilisp testrunner. sys.path already picks up the empty entry, and adding an additional init.py doesn't help:

It's harder to test this, but I didn't add the path fixes to basilisp test (oops!) so it could still be path related. At least as a short term fix.

The tests were mainly run using pytest on the command line, so it’s not specific to basilisp test. That was just to confirm it fails there too, so we can ignore basilisp test as the primary source for now I think, unless I'm missing something fundamental (like , pytest calling into basilisp test?).

I think we can set this aside for now since the workaround of adding an empty tests/conftest.py works. I'll try to look into the cause at some point.

thanks

@chrisrink10
Copy link
Member

@ikappaki I pulled down the repo to try to understand the issue. I'm noticing that when I do add the "" empty path to the test subcommand, that tests will run so long as you retain an empty tests/__init__.py:

tests
├── __init__.py
└── issuetests
    └── issue_test.lpy
=================================================================================================================================== test session starts ====================================================================================================================================
platform darwin -- Python 3.12.1, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/christopher/Projects/basilisp-issue-1069
configfile: pyproject.toml
plugins: basilisp-0.2.3
collected 1 item

tests/issuetests/issue_test.lpy .                                                                                                                                                                                                                                                    [100%]

==================================================================================================================================== 1 passed in 0.01s =====================================================================================================================================

I'm going to push a PR that makes the small change to the tests subcommand and updates the documentation to include this requirement.

I'd like to fix this correctly in the future, but will need to think a bit more on how to do that.

@ikappaki
Copy link
Contributor Author

ikappaki commented Sep 25, 2024

@ikappaki I pulled down the repo to try to understand the issue. I'm noticing that when I do add the "" empty path to the test subcommand, that tests will run so long as you retain an empty tests/__init__.py:

Hi @chrisrink10, thanks for looking into this.

It works great now! The key piece I was missing was the #1075 fix that ensures basilisp test properly passes its arguments to pytest. Before this fix, I thought basilisp test was mainly a convenience for basic scenarios, and that we still had to rely on pytest for comprehensive testing with its wide range of command line options.

Now, I understand that testing should indeed be driven through basilisp test.

I have two further suggestion, if you don't mind.

  1. Could you highlight this more prominently int he documentation, perhaps in the testing section?
  2. Would it be possible to display pytest's command-line help from basilisp test -h? Currently, users would have to switch back to pytest to find out which arguments are accepted by basilisp test, which feels a bit unintuitive.
(basilisp-pprint-py3.11) PS C:\clj\basilisp-pprint> basilisp test -h
usage: basilisp test [-h] [--generate-auto-inlines [GENERATE_AUTO_INLINES]] [--inline-functions [INLINE_FUNCTIONS]]
                     [--warn-on-arity-mismatch [WARN_ON_ARITY_MISMATCH]]
                     [--warn-on-shadowed-name [WARN_ON_SHADOWED_NAME]] [--warn-on-shadowed-var [WARN_ON_SHADOWED_VAR]]
                     [--warn-on-unused-names [WARN_ON_UNUSED_NAMES]]
                     [--warn-on-non-dynamic-set [WARN_ON_NON_DYNAMIC_SET]]
                     [--use-var-indirection [USE_VAR_INDIRECTION]]
                     [--warn-on-var-indirection [WARN_ON_VAR_INDIRECTION]]
                     [--include-unsafe-path [INCLUDE_UNSAFE_PATH]] [-p INCLUDE_PATH]
                     [--data-readers-entry-points [DATA_READERS_ENTRY_POINTS]] [--disable-ns-cache [DISABLE_NS_CACHE]]
                     [--enable-logger [ENABLE_LOGGER]] [-l LOG_LEVEL]
                     [--emit-generated-python [EMIT_GENERATED_PYTHON]]

Run tests in a Basilisp project.

positional arguments:
  args                  arguments passed on to Pytest
...

Another related thought: would it be possible to provide both brief and verbose help optoins for the CLI commands? at the moment, the output is cluttered with numerous compiler, runtime and debugging arguments, many of which would not be relevant when you are just looking for testing relating options, for example.

I'm going to push a PR that makes the small change to the tests subcommand and updates the documentation to include this requirement.

I'd like to fix this correctly in the future, but will need to think a bit more on how to do that.

I'm very satisfied with the current fix, as things are now much clearer! Thanks again.

@chrisrink10
Copy link
Member

chrisrink10 commented Sep 25, 2024

I have two further suggestion, if you don't mind.

  1. Could you highlight this more prominently int he documentation, perhaps in the testing section?

Seems reasonable!

  1. Would it be possible to display pytest's command-line help from basilisp test -h? Currently, users would have to switch back to pytest to find out which arguments are accepted by basilisp test, which feels a bit unintuitive.

I'm not sure if there's an easy way to do this that wouldn't be really ugly, but running basilisp test -- -h emits the Pytest help text. I think we could definitely make it clearer that anything past -- goes to Pytest.

Another related thought: would it be possible to provide both brief and verbose help optoins for the CLI commands? at the moment, the output is cluttered with numerous compiler, runtime and debugging arguments, many of which would not be relevant when you are just looking for testing relating options, for example.

The main reason I didn't do this was because short arguments become non-mnemonic rather quickly and there are only so many letters available. If you had any suggestion for how to handle that, I'd certainly be willing to consider it.

@ikappaki
Copy link
Contributor Author

  1. Would it be possible to display pytest's command-line help from basilisp test -h? Currently, users would have to switch back to pytest to find out which arguments are accepted by basilisp test, which feels a bit unintuitive.

I'm not sure if there's an easy way to do this that wouldn't be really ugly, but running basilisp test -- -h emits the Pytest help text. I think we could definitely make it clearer that anything past -- goes to Pytest.

I was not familiar with the -- convention, and even if I were, I would not have immediately thought of using -- -h, as simple as it is😅. It would definitely be helpful to mention this in the help output.

Another related thought: would it be possible to provide both brief and verbose help optoins for the CLI commands? at the moment, the output is cluttered with numerous compiler, runtime and debugging arguments, many of which would not be relevant when you are just looking for testing relating options, for example.

The main reason I didn't do this was because short arguments become non-mnemonic rather quickly and there are only so many letters available. If you had any suggestion for how to handle that, I'd certainly be willing to consider it.

My apologies for the confusion, I misused the terminology. What I had in mind was that -h would only return the specific options for the command. If the user wanted the full list of options, they could it request it separately, with something like -H or -hh or --uberhelp or (even) --help or something else along these lines. I am not sure how feasible these suggestions are though with the currently utilized command line options library.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:testing Issue pertaining to tests/testing issue-type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants