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

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

Closed
chrisgervang opened this issue Jan 12, 2020 · 18 comments · Fixed by #24384
Closed
Assignees
Labels
area-testing feature-request Request for new features or functionality needs proposal Need to make some design decisions

Comments

@chrisgervang
Copy link

Environment data

  • VS Code version: 1.41.0
  • Extension version (available under the Extensions sidebar): 2020.1.57204
  • OS and version: MacOS 10.14.6
  • Python version (& distribution if applicable, e.g. Anaconda): 3.7
  • Type of virtual environment used (N/A | venv | virtualenv | conda | ...): venv
  • Relevant/affected Python packages and their versions: pytest-5.2.2
  • Jedi or Language Server? (i.e. what is "python.jediEnabled" set to; more info How to update the language server to the latest stable version #3977): Language Server

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

args.splice(0, 0, '--rootdir', options.workspaceFolder.fsPath);

const options: TestRunOptions = {
workspaceFolder: this.workspaceFolder,
cwd: this.rootDirectory,

constructor(workspaceFolder: Uri, rootDirectory: string, serviceContainer: IServiceContainer) {
super(PYTEST_PROVIDER, Product.pytest, workspaceFolder, rootDirectory, serviceContainer);

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 (ViewOutput, 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 chrisgervang added triage-needed Needs assignment to the proper sub-team bug Issue identified by VS Code Team member as probable bug labels Jan 12, 2020
@chrisgervang
Copy link
Author

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

@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Jan 13, 2020
@ericsnowcurrently
Copy link
Member

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

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Jan 14, 2020

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
Copy link
Member

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.

@ericsnowcurrently ericsnowcurrently added the info-needed Issue requires more information from poster label Jan 14, 2020
@chrisgervang
Copy link
Author

chrisgervang commented Jan 14, 2020 via email

@ericsnowcurrently
Copy link
Member

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.

@ericsnowcurrently ericsnowcurrently removed their assignment Jan 14, 2020
@ericsnowcurrently ericsnowcurrently added area-testing needs decision feature-request Request for new features or functionality and removed info-needed Issue requires more information from poster triage bug Issue identified by VS Code Team member as probable bug labels Jan 14, 2020
@luabud luabud added needs proposal Need to make some design decisions and removed needs decision labels Feb 12, 2020
@luabud
Copy link
Member

luabud commented Feb 12, 2020

may be related: #7176

@chrisgervang
Copy link
Author

chrisgervang commented Feb 12, 2020

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
Copy link

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
Copy link

This also feels related to #8678.

@bhack
Copy link

bhack commented May 28, 2020

Any news on this?

@dcrystalj
Copy link

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
Copy link

niedakh commented Oct 15, 2020

Any news on this? :)

@rndev-io
Copy link

related to #12538

@ProLoser
Copy link

ProLoser commented Nov 6, 2020

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

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

@bhrutledge
Copy link

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 #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
Copy link

bhrutledge commented Nov 2, 2021

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.

export function pytestGetTestFolders(args: string[]): string[] {
const testDirs = getOptionValues(args, '--rootdir');
if (testDirs.length > 0) {
return testDirs;
}

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

// if user has provided `--rootdir` then use that, otherwise add `cwd`
if (testArgs.filter((a) => a.startsWith('--rootdir')).length === 0) {
// Make sure root dir is set so pytest can find the relative paths
testArgs.splice(0, 0, '--rootdir', options.workspaceFolder.fsPath);
}

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

const options: TestDiscoveryOptions = {
workspaceFolder: workspace.uri,
cwd:
settings.testing.cwd && settings.testing.cwd.length > 0
? settings.testing.cwd
: workspace.uri.fsPath,
args: settings.testing.pytestArgs,
ignoreCache: true,
token,
};
// Get individual test directories selected by the user.
const testDirectories = pytestGetTestFolders(options.args);
// Set arguments to use with pytest discovery script.
const args = runAdapter(['discover', 'pytest', '--', ...preparePytestArgumentsForDiscovery(options)]);

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:

return this.runner.runTests(
testRun,
{
workspaceFolder: workspace.uri,
cwd:
settings.testing.cwd && settings.testing.cwd.length > 0
? settings.testing.cwd
: workspace.uri.fsPath,
token,
args: settings.testing.pytestArgs,
},
this.idToRawData,

@eleanorjboyd
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-testing feature-request Request for new features or functionality needs proposal Need to make some design decisions
Projects
None yet
Development

Successfully merging a pull request may close this issue.