Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Virtualenv locator #13893
Virtualenv locator #13893
Changes from all commits
a4d4e3b
4e85c72
96a8b79
143020e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orpath.win32
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this test depends on the behavior of the
path
module. If in the rest of the code we letpath
do what ever it does, then in the test we should letpath
do whatever it does. we are not testingpath
here, and the fact thatpath
gets used anywhere at all is an internal implementation detail that this test should not care about.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inpath
you'd want to know about it). Are they brittle? (Here, over-reliance on implementation details would make the test brittle - and stubbingpath
would be an example of that).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, these are the case that I believe are appropriate for stubbing:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it wasn't clear, I'm not blocking the PR on this. 😄