Add API to identify pipenv

[karrtikr]

• 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!).
• Appropriate comments and documentation strings in the code.
• Has sufficient logging.
• Has telemetry for enhancements.
• Unit tests & system/integration tests are added/updated.
• Test plan is updated as appropriate.
• package-lock.json has been regenerated by running npm install (if dependencies have changed).
• The wiki is updated with any design decisions/details.

[codecov-commenter]

Codecov Report

Merging #13762 into main will increase coverage by 0.03%.
The diff coverage is 92.10%.

@@            Coverage Diff             @@
##             main   #13762      +/-   ##
==========================================
+ Coverage   59.84%   59.87%   +0.03%     
==========================================
  Files         695      696       +1     
  Lines       38402    38473      +71     
  Branches     5517     5534      +17     
==========================================
+ Hits        22981    23037      +56     
- Misses      14240    14250      +10     
- Partials     1181     1186       +5     

Impacted Files	Coverage Δ
.../pythonEnvironments/common/externalDependencies.ts	66.66% <72.72%> (-6.67%)	⬇️
...nments/discovery/locators/services/pipEnvHelper.ts	95.16% <95.16%> (ø)
...pythonEnvironments/common/environmentIdentifier.ts	88.88% <100.00%> (+2.22%)	⬆️
src/client/datascience/crossProcessLock.ts	79.41% <0.00%> (-11.77%)	⬇️
src/client/common/utils/platform.ts	60.00% <0.00%> (-8.00%)	⬇️
src/client/linters/pydocstyle.ts	86.66% <0.00%> (-2.23%)	⬇️
src/client/datascience/debugLocationTracker.ts	76.56% <0.00%> (-1.57%)	⬇️
src/client/common/process/proc.ts	14.49% <0.00%> (-0.73%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b21b2e1...1a5c76f. Read the comment docs.

[karthiknadig]

Some minor changes. This looks good to me.

[ericsnowcurrently]

Is this existing code that you copied from somewhere (if so, where?) or is it all new?

Also, I'd expect pipenv-related stuff to live next to the locator (rather than in src/client/pythonEnvironments/common), especially since the locator is the only thing that should be using these helpers.

[karrtikr]

@karthiknadig I misunderstood what the .project file is supposed to carry. It seems it always carries the folder which has the Pipfile. So we need not look into parent directories.

However we'll need to look into parent directories for isPipenvEnvironmentRelatedToFolder method, which is not related to the identifier itself, but will be used here.

vscode-python/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts

Lines 40 to 48 in 08e4074

	// Activate using `pipenv shell` only if the current folder relates pipenv environment.
	const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined;
	if (
	workspaceFolder &&
	interpreter.pipEnvWorkspaceFolder &&
	!this.fs.arePathsSame(workspaceFolder.uri.fsPath, interpreter.pipEnvWorkspaceFolder)
	) {
	return;
	}

When we get rid of pipEnvWorkspaceFolder property, we'll need this method which looks into parent directories. So I'm not removing it.

[karrtikr]

@ericsnowcurrently

Is this existing code that you copied from somewhere (if so, where?) or is it all new?

Most of it's new which are followed from spikes on Environment identifer and Pipenv locator. Some logic is borrowed from existing PipEnvService locator.

Also, I'd expect pipenv-related stuff to live next to the locator (rather than in src/client/pythonEnvironments/common), especially since the locator is the only thing that should be using these helpers.

Quoting #13589 (comment),

Since we don't have the templates for the (new) locators ready. I am putting them here for now. My plan is to move the isConda, isWindowsStore, etc, into respective locators.

UPDATE: I moved them to pipEnvHelper.ts for now.

[ericsnowcurrently]

I've left a few comments related to correctness, as well as some related to readability.

[karthiknadig]

For the tests in the PR, I have a PR coming up that will separate the functional and unit tests.
@karrtikr For now, I would address the code specific concerns. I labeled the file wrong when I created the tests. environmentIdentifier.unit.tests.ts should have been named environmentIdentifier.functional.tests.ts functional tests, since it runs against env layouts using filesystem calls.

[karrtikr]

@ericsnowcurrently There was confusion regarding the types of test to have, the functional and unit tests were mixed earlier. I was following the pattern which led to this mess, sorry. I will add more unit tests as you suggested and will separate the two.

[karrtikr]

@ericsnowcurrently I have added the tests and additional comments which may help. Please have a look again.

[ericsnowcurrently]

Thanks @karrtikr. I'm glad you're going to add unit tests too. Having both kinds of test is valuable.

FWIW, here's some insight on testing that you might find relevant:

Functional tests are definitely important, especially to validate actual behavior where we integrate with external resources/libs. At the same time, unit tests (where we check the logic of just the "unit" under test, w/o deps) are where we aim for branch (& line) coverage. Both are essential for providing confidence in the correctness of our code.

Both kinds of tests provide value. Functional tests check that our assumptions about integration are correct, while unit tests check that our code does what we meant to do. Functional tests are closer to actual user experience, while unit tests make it easier to pinpoint where bugs are. Functional tests serve well to validate higher-level objectives, while unit tests can help us track our progress in implementing those high-level objectives (e.g. TDD). Functional tests can help us identify the critical path cases, while unit tests can help us identify code paths we hadn't anticipated. Both give important early feedback on public API and on our design, as tests are the first place we actually write code that uses that API. However, we get different insight on that API from functional tests vs. unit tests.

On the flip side, both kinds of test have costs and downsides. Running functional tests is typically much slower than with unit tests, while with unit tests it is easy to bake in invalid assumptions about dependencies. Readable functional tests that are also correct can be hard to write, while unit tests can be fragile (in the face of code changes) if you are not careful. With functional tests you simply can't test all branches of the code (e.g. many failure cases), while with unit tests you can end up with a false sense of security about coverage.

All that is why it is important to have both kinds of test. They complement each other. Having one without the other leaves a project open to a greater likelihood of bugs.

[ericsnowcurrently]

A few more suggestions.

[karrtikr]

@ericsnowcurrently Thanks for the reminder! Please review again

[ericsnowcurrently]

Thanks for working through all my review comments. I appreciate your willingness to listen.

As far as correctness goes, I think you've got it right now. There's only the question of matching the env dir name exactly in getPipfileIfGlobal(), which isn't a major blocker (though I think we should fix it now).

The remainder of my comments relate to readability or other relatively cosmetic (though important) things. At this point I think I've identified all the concerns I have with the PR. So once you've addressed all the open comments, this PR should be good to go.

Thanks again!

[gramster]

Rather than creating these artifacts here, could you use an in-memory filesystem fake? That way the tests that use these can create these (e.g. from declarative JSON/YAML) and it would be clear what the test is doing. See for example https://dev.to/julienp/node-js-testing-using-a-virtual-filesystem-as-a-mock-2jln

[karrtikr]

Yes. There're other such tests as well which has already went in. We'll address fixing all these tests separately.

[karrtikr]

@ericsnowcurrently All the testing concerns will be addressed by Karthik in separate PR. Please approve if you find everything else okay.

[ericsnowcurrently]

LGTM as long as the matter of testing an "internal" function is resolved (one way or another).

Thanks for making the effort to work through all my review comments. It means a lot to me that you are always so willing to engage (and to push back when you disagree 😄)!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.1% Duplication