Virtualenv locator #13893
Virtualenv locator #13893
Conversation
Codecov Report
@@ Coverage Diff @@
## main #13893 +/- ##
==========================================
- Coverage 59.87% 59.86% -0.02%
==========================================
Files 693 694 +1
Lines 38348 38358 +10
Branches 5515 5516 +1
==========================================
+ Hits 22961 22962 +1
- Misses 14207 14213 +6
- Partials 1180 1183 +3
Continue to review full report at Codecov.
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
Single workspace and smoke tests are failing with |
src/client/pythonEnvironments/discovery/locators/services/virtualenvLocator.ts
Show resolved
Hide resolved
src/client/pythonEnvironments/discovery/locators/services/virtualenvLocator.ts
Show resolved
Hide resolved
src/client/pythonEnvironments/discovery/locators/services/virtualenvLocator.ts
Show resolved
Hide resolved
| const envRoot = path.join('path', 'to', 'env'); | ||
| const interpreter = path.join(envRoot, 'python'); |
There was a problem hiding this comment.
If it's a unit test then there should be no dependence on the behavior of the path module. Instead we would either stub that (or the functions) out or we would directly use one of the OS-specific implementations (path.posix or path.win32).
There was a problem hiding this comment.
I don't see how this test depends on the behavior of the path module. If in the rest of the code we let path do what ever it does, then in the test we should let path do whatever it does. we are not testing path here, and the fact that path gets used anywhere at all is an internal implementation detail that this test should not care about.
There was a problem hiding this comment.
Actually, functional testing is testing that the system as a whole functions as intended. So acceptance tests for user stories would be functional tests. Unit tests can test at levels of abstraction that are not the same as functional tests. But that does not mean that all dependencies should be stubbed. Stub dependencies that are stateful or have performance implications. Don't stub things just because they are dependencies.
You should always be focused on outcomes. Are the tests hermetic? Are they fast? (Those two would be reasons to stub stateful or slow dependencies). Do they find bugs? (I'd argue here that stubbing path is not going to help you find more bugs - and in fact if a bug was introduced in path you'd want to know about it). Are they brittle? (Here, over-reliance on implementation details would make the test brittle - and stubbing path would be an example of that).
There was a problem hiding this comment.
FWIW, these are the case that I believe are appropriate for stubbing:
- stateful shared dependencies
- slow dependencies
- non-deterministic components
- resource-intensive components
- hard to initialize/configure components
- components where you want to generate boundary test values that would be hard to create with the real code
- in TDD, stubbing dependencies that don't yet exist (until they do)
There was a problem hiding this comment.
Thanks. That's good info. My only concern is that, in general, we do not know if a dependency is stateful unless that project promises it is (and we trust them) or we inspect the code (and assume it doesn't become stateful later). FWIW, I agree that in the case of path we have a high level of confidence that it is neither stateful nor slow.
I guess my question is if it's more valuable to be consistent about strongly separating external dependencies or to allow exceptions. On the one hand, using test doubles adds complexity to tests, has a tendency to couple tests more tightly to code (making them more fragile), and moves them a step further from the actual runtime behavior. On the other hand, it means we don't have to spend the time to figure out (or guess) if a dependency needs to be stubbed out, nor do we have to worry about that behavior changing in the future.
So the way I see it, having a consistent separation from external dependencies allows us to always focus just on the logic of the unit under test, which from my perspective is the essential purpose of unit tests. For me, when we run into the downsides of test doubles (for external dependencies), it is almost always an indicator that the unit-under-test is trying to do too much, rather than the test doubles being the problem.
Regardless, I don't think this is a critical issue, especially since I doubt there will be many stateless+fast external dependencies. So this isn't something we need to spend much more time on. However, I do want to have a better sense of the actual tradeoffs. 😄
There was a problem hiding this comment.
In case it wasn't clear, I'm not blocking the PR on this. 😄
For #12020
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).