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

Proposal: Enable separate configs per run/discovery/debug with testing #21845

Open
eleanorjboyd opened this issue Aug 18, 2023 · 41 comments
Open
Assignees
Labels
area-testing needs proposal Need to make some design decisions

Comments

@eleanorjboyd
Copy link
Member

eleanorjboyd commented Aug 18, 2023

The following outlines a proposal for how to handle testing args.

Submitted issues related to this problem:

Current design:
Currently, to pass any command line args for test run/discovery via the extension there is a single setting per test type (one for unittest, one for pytest). The two settings are python.testing.pytestArgs: [] and python.testing.unittestArgs: [] which take a string array of command line args. The args are then processed by the extension to remove some not applicable to the current action (removing run args during discovery and discovery args during run) but this is buggy and hard to maintain. Additionally to debug a debug config can be created in the launch.json.

Issues with current design:

  1. With only a single place to put args for each test type, a user cannot distinguish between which command line args they want to include during test run, discovery, coverage, or debug.
  2. Since pytest does not allow some args to be included during with the arg --collect-only (used to distinguish run from discovery by the extension) these args are parsed out by the extension before sending the request. This means custom args are removed from test args and generally it creates a buggy experience that is hard to maintain with the breadth of args users would like to pass into their tests.
  3. There is no straightforward solution to providing environment variables while running and discovering tests.

Proposed new design:
Create new custom launch configs that can be setup for test run and discovery. Also, increase compatibility with debug launch configs to make sure these work in tandem. The launch config would be set up something like this:

    {
      "name": "Python: pytest run",
      "type": "python-pytest",
      "request": "launch",
      "args": ["", ""],
      "env": {
        "PYTHONPATH": "${workspaceFolder}"
      }
    },
    {
      "name": "Python: pytest discover",
      "type": "python-pytest",
      "request": "launch",
      "args": ["", ""],
      "env": {
        "VAR": "true"
      }
    }

The two particular fields "args" and "env" would provide functionality useful to users. The rest would be default values.

The new launch config types that would be added would be:

  1. Python: pytest run
  2. Python: pytest discover
  3. Python: unittest run
  4. Python: unittest discover
    The names of each one are up for discussion, please provide feedback below!

Next a way to set up these launch configs would be introduced to the extension. The initial steps would be:
image
add configuration -> python -> python config menu
Two possible options for how to display the testing config options would be:
A: to have another submenu called something like "testing" which, when opened, lists the 4 config options above
B: have the 4 config options sit in the current python submenu.

(I think option A is better for clarification even though it requires one more menu clickthrough).

additionally the button "configure tests" found on the test explorer would be changed so it routed the user to a menu of the launch config options for testing (similar to option A).
image

Concerns with proposed design:

  1. The use of launch configs may be harder for new / beginner users.
  2. The discoverability of launch configs is harder.
  3. The transition from a setting to using a config could be complicated/confusing for current users.

Possible ways to address above concerns

  1. help beginner users
    a. Increase documentation and provide specific examples of launch configs.
    b. provide default values for all fields in the launch config (this is already planned so the launch config should be runnable upon creation without edits meaning no args or env vars need to be provided)
  2. discoverability
    a. There is already a larger discussion happening on how we can improve this, switching to configs for testing provides even more reason to tackle this problem
    b. switching the button "configure tests" to the new config flow is very discoverable since that is already the workflow
  3. transition
    a. We can have a deprecation notification regarding the setting and show it to all those still using the setting for test args
    b. It should be simple to take someone's test args in their settings and turn that into a launch config so we could offer a button that handles this for the user to switch them over (drawback is we would be setting discovery and run to the same args but the user could see this then go change it)

Notes on Debugging:
Less investigation has been done into test debugging as we already offer a similar launch config. Further analysis should be done to see what is missing in this flow and how to improve it to align with test configs once a design is decided on.

Action Items:
We are looking for feedback on this proposal so please comment below any thoughts. Thanks!

@eleanorjboyd eleanorjboyd added area-testing needs proposal Need to make some design decisions needs community feedback Awaiting community feedback labels Aug 18, 2023
@eleanorjboyd eleanorjboyd self-assigned this Aug 18, 2023
@github-actions

This comment was marked as off-topic.

@roblourens
Copy link
Member

The "name" of a launch config is for the name that the user picks, it shouldn't be used to change the behavior of the launch config. Also what does "discovery" involve? We shouldn't use launch configs for anything that isn't basically running/debugging a program.

@roblourens
Copy link
Member

Another suggestion, I don't know how much complexity is behind the "python-pytest" debug adapter, but if the point is just to invoke the python DA with some args, another option would be to add a config snippet to help users figure out how to write their pytest launch configs. An example of this is the snippets that we have for js-debug to help users write launch configs that start an npm script.

@eleanorjboyd
Copy link
Member Author

Yes, I forgot to update the "name" of the launch config to represent that as a user input and instead focus on using "type" to change the behavior.

discovery involves running pytest discovery. It is equivalent to running "python -m pytest --collect-only" from the command line. So it is running python code if that is the concern.

I will look into the js-debug snippet! Thanks

@eleanorjboyd
Copy link
Member Author

adding #18778 to the related issues as this discusses limitations with current setup.

@roblourens
Copy link
Member

roblourens commented Aug 23, 2023

discovery involves running pytest discovery

Yes- and what is pytest discovery? 😄

and instead focus on using "type" to change the behavior.

I would say this also isn't what I expect- "type" should indicate which debug adapter is being used, and this should just be "pytest" or even "python" or something like that, and then there should be some other argument that changes the behavior.

@roblourens
Copy link
Member

In that issue, it sounds like it's discussing limitations with launch configs, not settings? I don't understand what Karthik means when he says

We currently only read this from the launch.json. We could expand the scope of where we read it from, but that has some things that we have not decided yet.

@eleanorjboyd
Copy link
Member Author

in #18778? I tagged it since resolving this args issue would then remove the setting entirely so it would simplify this process since the launch config would be the only place to reference. I mis-attributed it to "limitations" it is more like this issue would provide clarity to the issue brought up in #18778.

@eleanorjboyd
Copy link
Member Author

eleanorjboyd commented Aug 29, 2023

Yes- and what is pytest discovery? 😄

During discovery pytest goes through and discovers all files that fit the file format for test files and finds tests within those files (both test classes and just functions). It also should read markers to determine which tests to return. So it isn't executing any of the users test code.

I would say this also isn't what I expect- "type" should indicate which debug adapter is being used, and this should just be "pytest" or even "python" or something like that, and then there should be some other argument that changes the behavior.

gotcha yeah that makes sense

@roblourens
Copy link
Member

During discovery pytest goes through and discovers all files that fit the file format for test files and finds tests within those files (both test classes and just functions)

This doesn't sound like a good match for a debug config then, if I'm not going to be setting breakpoints and debugging my code. It sounds more like a task perhaps? Or just something that the extension does, configured by some settings.

@eleanorjboyd
Copy link
Member Author

@brettcannon, @karthiknadig, @luabud : thoughts on @roblourens comment?

@luabud
Copy link
Member

luabud commented Aug 30, 2023

The thing is that to configure the debugger to debug pytest tests, one needs to create a launch.json file. Given that, I think it makes sense to migrate how users can configure how to run pytest tests in launch.json too.
But the whole issue is that users would like to pass different configurations for pytest discovery, which needs to be done prior run/debug. If the config for running and debugging pytest tests are in launch.json, isn't it a bit weird to have the configuration for discovery elsewhere?

@luabud
Copy link
Member

luabud commented Aug 30, 2023

The other thing to call out is .env files -- using launch.json allow users to have different pytest configs with different environment variables per run, which is harder to do with settings

@brettcannon
Copy link
Member

I guess my question is is launch.json is just for debugging why isn't it named debug.json? 😉 I personally view launch.json as the place to configuring launching your code which is traditionally for the debugger. As well, I views tasks.json as a way to run something in the terminal that we don't have extension support for and to parse the output (which this isn't doing; it's directly tying into things).

The other thing is that while you might not use the debugger every time, you may want to use the debugger when running the tests, and so it isn't a disconnect, I think, to use launch.json this way. And since discovery and running are tied together, I think putting one somewhere else just because you typically don't debug with it seems like we're putting purity over practicality. I.e., what Luciana said. 😁

Do we know how other testing extensions handle configuration for debugging compared to running? And we can bring this up in the Pacific stand-up if necessary.

@roblourens
Copy link
Member

I still don't really understand what discovery is or what the user flow is for this scenario. I haven't used a testing framework that has discovery as a separate step. What is the output of discovery?

So if I want to debug my tests, I'm going to run a "discovery" launch config, wait for that to finish, then run the "run tests" launch config?

@roblourens
Copy link
Member

different configurations for pytest discovery, which needs to be done prior run/debug

Sounds like a prelaunch task?

@brettcannon
Copy link
Member

I still don't really understand what discovery is or what the user flow is for this scenario. I haven't used a testing framework that has discovery as a separate step. What is the output of discovery?

What tests are known to pytest. We need to do this to get the list of tests to display in the test explorer. Otherwise we would have to execute the tests in order to know which tests exist. How does js-debug know what tests to list in the test explorer?

So if I want to debug my tests, I'm going to run a "discovery" launch config, wait for that to finish, then run the "run tests" launch config?

You won't directly, but yes because we need to know about the test in order for you to be able to run it, debugger or not. Now if you're asking about when you run your tests after we know the test exists, then the discovery step is skipped as we already have a way at that point to tell pytest "run this specific test you told us about previously".

Sounds like a prelaunch task?

If by "prelaunch" you mean "before running tests", then yes. But if that's a push for using tasks then I still think that disassociation is going to confuse people. To a Python developer, test discovery is just a flag/command to the test runner, not a separate task (it essentially a dry run of tests where you are saying, "what tests can you find?", and skipping the actual execution of the tests).

@roblourens
Copy link
Member

How does js-debug know what tests to list in the test explorer?

There's no connection between js-debug and the test explorer. Our self-host test provider that we use for the vscode repo https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-selfhost-test-provider creates a filewatcher for our test files, and parses them when they are added or changed. It's automatic there, I don't know about other test provider extensions. I wouldn't expect to run a launch config to populate the test explorer, that feels backwards.

I didn't realize that this is about the test explorer. So is the expected user flow like this?

  1. Run the "discovery" launch config
  2. Tests show up in the test explorer
  3. Debug a test

And does 3 happen via the Test Explorer UI or the proposed Python: pytest run launch config?

@brettcannon
Copy link
Member

parses them when they are added or changed.

pytest and unittest handle the test discovery, so we need to have those tools tell us what they find.

  1. Run the "discovery" launch config

We run the discovery launch config; there's no manual step here beyond any custom configuration from the user.

This issue is just about configuration; the user doesn't perform any extra action for testing because of this and this is only for special cases where users need to specify extra details.

@roblourens
Copy link
Member

We run the discovery launch config

Launch configs are usually for users, they don't get run automatically in the background by an extension. When you do that, the debug toolbar will show up and the statusbar will turn orange and that will be spooky if it happens by itself.

@cameronbrill
Copy link

hi @eleanorjboyd! I love this proposal, it would fix an issue my org has with our test suite (we use pytest-xdist to parallelize test runs, but this breaks when we try debugging tests in VS Code). Is there a timeline for when this will be supported?

@brettcannon
Copy link
Member

Is there a timeline for when this will be supported?

Eleanor is busy w/ non-test stuff this month, but the plan is to pick it back up in April.

@alenzo-arch
Copy link

+1 for this. The collection thing has been really frustrating. Not just that it doesn't work with some args, but also that if you have coverage enabled like so, it overwrites the coverage report. Not critical but it is annoying if you have it setup to discover on save.

The debugging portion of this is a deal breaker. It's working again now, but my issue #22820 is an example of how the current solution is brittle.

"python.testing.pytestArgs": [
        "--cov",
        "--cov-report=html",
        "-vvv",

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

No branches or pull requests

9 participants