basilisp-lang / basilisp

A Clojure-compatible(-ish) Lisp dialect targeting Python 3.8+
https://basilisp.readthedocs.io
Eclipse Public License 1.0
258 stars 7 forks source link

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

Open ikappaki opened 3 weeks ago

ikappaki commented 3 weeks ago

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)
  2. 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)))

3. Run `pytest`, you will encounter the "No module named 'tests' error"
``` ps1
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 commented 3 weeks ago

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.

ikappaki commented 3 weeks ago

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

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago
  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