fix testrunner to handle relative entries in sys.path #1205
Conversation
ikappaki
force-pushed
the
issue/testrunner-relative-paths
branch
2 times, most recently
from
fc4792b to
8d53573
Compare
ikappaki
force-pushed
the
issue/testrunner-relative-paths
branch
from
8d53573 to
cd62cea
Compare
tests/basilisp/testrunner_test.py
Outdated
| print(f"\n\n--cmd--start: {' '.join(cmd)}") | ||
| print(f"\n\n--stdout--\n\n {result.stdout.strip()}") | ||
| print(f"\n\n--stderr--:\n\n {result.stderr.strip()}") | ||
| print(f"\n\n--cmd--end--: {' '.join(cmd)}\n\n") |
There was a problem hiding this comment.
Is this necessary or was this just for debugging?
There was a problem hiding this comment.
Not strictly necessary, as the following assert on stdout will indicate any issues. However, I found it helpful for debugging since it also displays stderr, which would otherwise be hidden. Let me know if you want it removed (or commented out).
There was a problem hiding this comment.
I would prefer if you could remove it. I don't want tests emitting stuff to stdout if it is avoidable.
| # I couldn't find a way to directly manipulate the pytester's | ||
| # `sys.path` with the precise control needed by this test, so we're | ||
| # invoking `basilisp test` directly as a subprocess instead ... | ||
| basilisp = shutil.which("basilisp") | ||
| cmd = [basilisp, "test"] | ||
| result = subprocess.run(cmd, capture_output=True, text=True, cwd=pytester.path) |
There was a problem hiding this comment.
Does it not use the same sys.path as the running executable? By which I mean are you not able to monkeypatch sys.path to the value you need?
There was a problem hiding this comment.
Unfortunately, there's no way to replicate the desired behavior exactly. We need a test case where sys.path starts with "", followed by the minimal system Python paths. I couldn't achieve this using pytester.runpytest with monkeypatch or pytester.runpytest_subprocess, as the latter always adds the test CWD to the path
|
|
||
| # Ensure the test module was loaded because it was directly | ||
| # relative to an entry in `sys.path`. | ||
| if module.__name__ != munge(str(ns)): |
There was a problem hiding this comment.
Does this cause problems with the changes introduced by #1176 ?
There was a problem hiding this comment.
Yes, and in particular it was breaking the test_ns_not_in_syspath test.
Consider the following file relative to the project directory:
./test/a/test_path.lpy, declared with the namespace a.test-path.
Running basilisp test will locate and load a.test-path due to "" in sys.path, but this is inconsistent because the namespace doesn't match the load path (it’s not test.a.test-path). Thus, an exception is thrown in this case.
(To make this work, the user must add the -p test option to the cli tool)
|
Hi @chrisrink10, Since this is a regression, preventing running Thanks |
https://github.com/basilisp-lang/basilisp/releases/tag/v0.3.7 |
Hi,
could you please review patch to fix a likely regression issue where the testrunner couldn't handle relative paths in
sys.path. It fixes #1204.This is likely a regression from #1176, where the update to the new module loading system caused relative paths to never match a fully qualified module name. The patch now expands relative paths to their absolute form before comparing them to the module path.
This issue was breaking the vanilla
basilisp testcommand, as it adds an empty string ("") entry tosys.path, which is interpreted as".". Since this is a relative path, it wasn’t handled by the logic, causing the project directory not to be recognized as the root of the Basilisp test modules.I’ve added a test that invokes
basilisp testdirectly as a subprocess. Unfortunately, I couldn’t find a way to control the pytestersys.pathentries with the precise control required for this test. Any counter suggestions are most welcome.Thanks