Derive module (test) names from ns declarations

[ikappaki]

Hi,

can you please review patch to derive module names for test namespaces directly from their top level ns declaration. It fixes #1155 and #1165.

The new approach improves upon the previous implementation by setting module names explicitly from the ns declaration at the top of the Basilisp files, instead of infering them from the file system structure using heuristics like checking for __init__.py files.

I've updated the testrunner tests accordingly to cover various setups and tested it with various projects with tests, including basilisp-blender and basilisp-kernel, and the hiccup port, which follows the typical clojure test structure. I've also updated the documentation to describe the default tests/ as well as the typical clojure test/ structure.

Since I don't have a full understanding of the Basilisp module loading system, I may have missed something, so any feedback or suggestions are welcome.

Lastly, I’ve introduced strict handling for loading test namespaces without an ns declaration: an exception will be thrown in such cases. This behavior can be relaxed if necessary, but I can't think of a scenario where a Basilisp test file would be missing an ns declaration at the top.

Thanks

Update1: I've also removed the following path deprecated parameter while at it:

tests/basilisp/testrunner_test.py: 13 warnings
  /home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/basilisp/contrib/pytest/testrunner.py:62: PytestRemovedIn9Warning: The (path: py.path.local) argument is deprecated, please use (file_path: pathlib.Path)
  see https://docs.pytest.org/en/latest/deprecations.html#py-path-local-arguments-for-hooks-replaced-with-pathlib-path
    def pytest_collect_file(  # pylint: disable=unused-argument

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

[chrisrink10]

Thank you for the submission. I'll be honest and just say that I don't really like this approach, but I also cannot think of a better one offhand. I think I might want to just leave this open for a bit while I consider how I want to proceed. There might need to be some reworking of the import logic to handle the issues you've filed.

This behavior can be relaxed if necessary, but I can't think of a scenario where a Basilisp test file would be missing an ns declaration at the top.

Part of the issue (for me) is that we're trying to support Clojure too and you personally have found dozens of cases where my expectations of how Clojure code does things was wildly incorrect. I'm hesitant to make assumptions about the ways people write Clojure given my track record so far.

[chrisrink10]

Does removing this work on older versions of PyTest (7, 8)? I was aware of the deprecation, but was not addressing it while we were supporting versions of PyTest that may have still required it.

[ikappaki]

It does indeed

$ poetry run pytest
=================================================== test session starts ===================================================
platform win32 -- Python 3.11.4, pytest-7.4.4, pluggy-1.5.0
rootdir: d:\src\basilisp
configfile: pyproject.toml
plugins: anyio-4.7.0, basilisp-0.3.4, pycharm-0.7.0
collected 3637 items

tests\basilisp\atom_test.py ........                                                                                 [  0%]
...

=============== 3632 passed, 5 skipped in 67.79s (0:01:07) =======

In addition, the link to the relevant page in the original warning message clearly states at the start that they started deprecating from v7: https://docs.pytest.org/en/latest/deprecations.html#py-path-local-arguments-for-hooks-replaced-with-pathlib-path

[ikappaki]

Thank you for the submission. I'll be honest and just say that I don't really like this approach, but I also cannot think of a better one offhand. I think I might want to just leave this open for a bit while I consider how I want to proceed. There might need to be some reworking of the import logic to handle the issues you've filed.

Sure, thanks. I was just breaking the ice here, but I lack the full picture and the direction. I do think the tests are valuable for capturing the requirements though, so it was nevertheless a good exercise.

Could you share why reading the namespace from the file might not be considered good practice? To me, it feels like the definitive source.

This behavior can be relaxed if necessary, but I can't think of a scenario where a Basilisp test file would be missing an ns declaration at the top.

Part of the issue (for me) is that we're trying to support Clojure too and you personally have found dozens of cases where my expectations of how Clojure code does things was wildly incorrect. I'm hesitant to make assumptions about the ways people write Clojure given my track record so far.

👍

[chrisrink10]

I don't think this PR is necessary anymore, right?

[ikappaki]

As better Implemented in #1176