Test discovery using Python pytest

[DonJayamanne]

For #4035.

This is a refactor of #4695.

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Has a news entry file (remember to thank yourself!)
• Has sufficient logging.
• Has telemetry for enhancements.
• Unit tests & system/integration tests are added/updated
• [n/a] Test plan is updated as appropriate
• [n/a] package-lock.json has been regenerated by running npm install (if dependencies have changed)
• [n/a] The wiki is updated with any design decisions/details.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@d2cc712). Click here to learn what that means.
The diff coverage is 33%.

@@           Coverage Diff            @@
##             master   #4795   +/-   ##
========================================
  Coverage          ?     62%           
========================================
  Files             ?     375           
  Lines             ?   14714           
  Branches          ?    1159           
========================================
  Hits              ?    9037           
  Misses            ?    5476           
  Partials          ?     201

[ericsnowcurrently]

Mostly LGTM. There are a few odds and ends to clean up. :)

[ericsnowcurrently]

I found a few things that I'd like addressed. Otherwise, LGTM.

FWIW, earlier reviews were incomplete; I stopped reviewing once I reached a critical mass of review comments. With this review I made it all the way through. :)

[ericsnowcurrently]

The adapter script has a certain set of args that are required and will be the same every time. I'd expect those to be generated as close to this exec() call as possible. Why are we leaving those args up to some other class?

I imagine the code here looking something like this:

const args = [DISCOVERY_FILE, 'discover', options.tool, '--', ...options.toolargs]
return execService.exec(args, spawnOptions);

[DonJayamanne]

Why are we leaving those args up to some other class?

Separation of concerns... creating args is one thing, and executing it another.

[ericsnowcurrently]

I'm not buying this. What are the concerns that need separating? The args that I'm talking about are intrinsic to the adapter script. They're fundamentally coupled. The corresponding code should be together. Other, tool-specific args certainly deserve separation. However, I'm not talking about those. :)

[DonJayamanne]

I'm keeping them separate as it's easier to manage.
Arguments need to get build/parsed, sepearately for PyTest, unit test and nose, and having them done in three separate places makes sense rather than having case statements in one file.
I guess we're going to have to agree to disagree, however lets discuss this in engineering meeting.

[ericsnowcurrently]

Thanks for sticking with this. Aside from a few open comments in my last review, LGTM. I'll assume you'll sort those out. :)

[DonJayamanne]

@ericsnowcurrently I've added two separate tests for the items you've suggested.

[WSLUser]

I'm still not getting pytest discovered from my Miniconda installation. I installed it with both conda and pip and either way, the extension doesn't discover it. I have it installed at the default path of C:\ProgramData\Miniconda3.

[DonJayamanne]

@WSLUser Please file an issue so we can help you.