search

microsoft / vscode-python

Python extension for Visual Studio Code
https://aka.ms/pvsc-marketplace
MIT License
4.26k stars 1.16k forks source link

[pytest] If present, "python.testing.cwd" expected to change `pytest --rootdir` arg value. #9553

Open chrisgervang opened 4 years ago

chrisgervang commented 4 years ago

Environment data

Expected behaviour

When running a test with a settings.json that contains the "python.testing.cwd": "root/dir" option should result in a pytest command that uses "root/dir" as the --rootdir argument. If it is null, then fallback to existing behavior: --rootdir ${workspaceFolder}.

This is necessary for my use case since my code is in a monorepo and loads test fixtures from disk using paths relative to a project root rather than the repo root.

For example, 

python [~]/.vscode/extensions/ms-python.python-2020.1.57204/pythonFiles/testing_tools/run_adapter.py discover pytest -- --rootdir root/dir -s --cache-clear [...testfolders]

jfyi, I've anonymized [~] and [...testfolders].

Actual behaviour

The "python.testing.cwd" setting has no visible effect on --rootdir. It is always the "workspace folder", which makes sense given the code I linked to below.

Relevant Code

https://github.com/microsoft/vscode-python/blob/2b6a8f2d439fe9d5e66665ea46d8b690ac9b2c39/src/client/testing/pytest/services/discoveryService.ts#L53

https://github.com/microsoft/vscode-python/blob/2b6a8f2d439fe9d5e66665ea46d8b690ac9b2c39/src/client/testing/pytest/main.ts#L50-L52

https://github.com/microsoft/vscode-python/blob/2b6a8f2d439fe9d5e66665ea46d8b690ac9b2c39/src/client/testing/pytest/main.ts#L19-L20

Steps to reproduce:

[NOTE: Self-contained, minimal reproducing code samples are extremely helpful and will expedite addressing your issue]

  1. Open a workspace folder containing python tests that are not in the root directory (i.e. a monorepo).
  2. Configure pytest to test a project directory. 2a. configure "python.testing.cwd" in settings.json to the project root.
  3. Discover tests and observe that all tests are discovered.
  4. Run all tests. 4a. Observe the test session starts and all tests pass that do not need to load fixtures from disk. 4b. Observe the output logs look like this:
python [~]/.vscode/extensions/ms-python.python-2020.1.57204/pythonFiles/testing_tools/run_adapter.py discover pytest -- --rootdir [workspaceFolder] -s --cache-clear [project dir]
============================= test session starts ==============================
platform darwin -- Python 3.7.6, pytest-5.2.2, py-1.8.1, pluggy-0.13.1
rootdir: [workspaceFolder]
plugins: doubles-1.5.3, cov-2.8.1

Logs

Output for Python in the Output panel (View→Output, change the drop-down the upper-right of the Output panel to Python)

XXX

Output from Console under the Developer Tools panel (toggle Developer Tools on under Help; turn on source maps to make any tracebacks be useful by running Enable source map support for extension debugging)

XXX
chrisgervang commented 4 years ago

Somewhat related to #9349 and #6881, but I don't believe I'd expect any auto detection.

ericsnowcurrently commented 4 years ago

@chrisgervang, thanks for getting in touch with us. We'll take a look as soon as possible.

ericsnowcurrently commented 4 years ago

Having looked this over, did you mean to file this as a bug or as a feature request? FWIW, I consider this a feature request. Just to be sure I understood, what you are asking for is that the "python.testing.cwd" setting be treated as the "root" directory against which the tests will be discovered/run. Is that correct?

Assuming yes, I see three options:

  1. the extension starts using "python.testing.cwd" as the test root dir (--rootdir for pytest, --top-directory for unittest)
  2. the extension adds a new setting to use as the test root
  3. the docs/help text are updated to be more clear
  4. nothing changes (not great)

If we do something about this then I expect it would be option 2. The reason is, "cwd" already has a specific meaning (essentially "the working directory of the executing program"). In the context of testing this often relates closely with the test root, which is part of the confusion here. (Sorry the help text for the setting, Optional working directory for tests. isn't clear.) The semantics of CWD for program execution need to remain, but may be distinct from the test root, so the best solution will likely be to add a new setting to explicitly define an alternate test root, something like "python.testing.testroot".

Oh, and what in particular connected the "python.testing.cwd" setting to pytest's --rootdir from your perspective?

ericsnowcurrently commented 4 years ago

Also, have you tried the multi-root workspace functionality of VS Code. You create a workspace and then "Add Folder to Workspace...". Consider adding each project root as a workspace root to see if that solves the problem for you.

chrisgervang commented 4 years ago

Gotcha, I agree with you that this is a feature request and #2 sounds like a good addition given there is an existing, different semantic for CWD.

It was the quoted language in docs that led to my confusion - I thought this was a bug and the expected behavior was to use CWD for —rootdir.

I haven’t tried the multiple workspace feature yet, but will give it a go. Thanks for the suggestion. Looking at the plugin code, I’m not sure how it’ll resolve multiple workspace folders down to one. Does it use the “closest” (down the tree) folder of all the workspace folders added? I’ll be interested to find out.

Thanks for looking into this!

On Tue, Jan 14, 2020 at 11:16 AM Eric Snow notifications@github.com wrote:

Also, have you tried the multi-root workspace functionality of VS Code. You create a workspace and then "Add Folder to Workspace...". Consider adding each project root as a workspace root to see if that solves the problem for you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/microsoft/vscode-python/issues/9553?email_source=notifications&email_token=AASY6273ODGS5PDTLSLV2OLQ5YFQFA5CNFSM4KFUZ4YKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI5Y7FA#issuecomment-574328724, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASY62ZLCINBWJ6HGMBGRI3Q5YFQFANCNFSM4KFUZ4YA .

ericsnowcurrently commented 4 years ago

Sounds good. We'll make a decision on the request.

As to multi-root workspace, the extension determines the current workspace based on the file in the active editor. Really it just calls the workspace.getWorkspaceFolder() function in VS Code's API. I'm not sure how it decides it several roots could match for a file.

luabud commented 4 years ago

may be related: #7176

chrisgervang commented 4 years ago

Thanks. I did work around this issue using the multi-root workspace functionality, and still interested in a decision. Feel free to close if the discussion has been moved.

bhrutledge commented 4 years ago

I just stumbled upon this. My pytest.ini (and Django project root) is 2 directories deep in my workspace folder, and tests are 4 directories deep. Due to the nature of our project, and my own workflow, I'm reluctant to split it up into a multi-root workspace.

So, 👍 for making the --rootdir option configurable. For now, I seem to be able to workaround this by adding the project root path to python.testing.pytestArgs (which feels like a hack).

bhrutledge commented 4 years ago

This also feels related to https://github.com/microsoft/vscode-python/issues/8678.

bhack commented 4 years ago

Any news on this?

dcrystalj commented 3 years ago

There is issue in our company we use old pytest v3 and giving --rootdir param is causing problems that we cannot use pytest discovery module. Please add option "python.testing.cwd": null to completely remove --rootdir parameter!

niedakh commented 3 years ago

Any news on this? :)

rusnasonov commented 3 years ago

related to #12538

ProLoser commented 3 years ago

Couldn't this be fixed by removing --rootdir from optionsWithArgsToRemove?

This way if people wish to override the arg they can.

bhrutledge commented 2 years ago

Revisiting this, because it's still not clear to me how to configure my project. I've put up a minimal project with the same layout at https://github.com/bhrutledge/vscode-pytest-workspace. The latest commit gets it working with a hack, but the commit history shows my other attempts to get it working.

@karthiknadig I think https://github.com/microsoft/vscode-python/pull/17509 helped with test discovery by defaulting --rootdir to cwd, but that doesn't seem carry over to running tests. It also seems to force the test discovery path to be the value of --rootdir, instead of the value provided in pytestArgs or the testpaths option in pytest.ini.

This is on the latest Insiders builds of VS Code and the Python extension.

bhrutledge commented 2 years ago

After some more digging, I think I've found two issues.

First, during test discovery, it seems that --rootdir takes precedence over any positional argments. It's not clear why this behavior exists, but it looks like it was added in June 2018 by @DonJayamanne, and moved in Aug 2021.

https://github.com/microsoft/vscode-python/blob/b6c0d3e19e4864273153cd639def8a6713ac02d6/src/client/testing/testController/pytest/arguments.ts#L137-L141

Second, during test running, --rootdir is set to ${workspaceFolder} if it's not set in pytestArgs:

https://github.com/microsoft/vscode-python/blob/b6c0d3e19e4864273153cd639def8a6713ac02d6/src/client/testing/testController/pytest/runner.ts#L89-L93

However, this is different than test discovery, which uses cwd or ${workspaceFolder}:

https://github.com/microsoft/vscode-python/blob/b6c0d3e19e4864273153cd639def8a6713ac02d6/src/client/testing/testController/pytest/pytestController.ts#L178-L193

It seems like this logic is already present in the options object, so this might just be using options.cwd instead of options.workspaceFolder.fsPath:

https://github.com/microsoft/vscode-python/blob/b6c0d3e19e4864273153cd639def8a6713ac02d6/src/client/testing/testController/pytest/pytestController.ts#L288-L299

eleanorjboyd commented 7 months ago

Hello! I have just found and caught up on this issue and wanted to get feedback following the rewrite for our testing infrastructure.

I do not think this feature request is fixed. The code now has --rootdir likely taking precedence because I do not do any editing to user args as they are getting passed in. Deciding to take the cwd over the -rootdir is a more opinionated decision for the extension to take as we are ignoring a user inputted value. There is obviously interest here but I am unsure of if there are people that use it the reverse way (expecting --rootdir to take precedence).

@luabud and @karthiknadig, we do not have a precedence set for which one is used if both are provided do we? What would the risks with adopting something like this be in terms of potentially breaking other users by changing the order of precedence.