Register or access old Conda locator only when not in experiment#15292
Conversation
Codecov Report
@@ Coverage Diff @@
## main #15292 +/- ##
======================================
- Coverage 64% 64% -1%
======================================
Files 560 560
Lines 26790 26834 +44
Branches 3862 3867 +5
======================================
+ Hits 17413 17436 +23
- Misses 8660 8682 +22
+ Partials 717 716 -1
|
| condaLocatorService = mock<ICondaLocatorService>(); | ||
| processLogger = mock(ProcessLogger); | ||
| experimentService = mock(ExperimentService); | ||
| when(experimentService.inExperiment(DiscoveryVariants.discoverWithFileWatching)).thenResolve(false); |
There was a problem hiding this comment.
What is the advantage of mocking false for one experiment and not the other, and of doing that vs mocking the inDiscoveryExperiment helper directly?
There was a problem hiding this comment.
It's because we're trying not to stub implementation details where we can, inDiscoveryExperiment in our case. And only stub where it's necessary like filesystem boundaries, env variables, etc.
What is the advantage of mocking false for one experiment and not the other
Actually I should be stubbing false for both the cases, will do.
There was a problem hiding this comment.
we're trying not to stub implementation details where we can
Since we're mocking/stubbing the implementation details of inDiscoveryExperiment here, if one day it changes we'll have to change all these tests even though they don't explicitly call this method?
There was a problem hiding this comment.
In this case it may not be much. But the advantage of that is supposed we access inExperiment API differently, without changing the overall behavior, the same tests would still hold up. In other words, tests are loosely coupled with the implementation.
There was a problem hiding this comment.
if one day it changes we'll have to change all these tests even though they don't explicitly call this method
And yes, but I think that's also the right thing to do. That way we would be aware of what impact this change is having. We can take this discussion offline at this point.
kimadeline
left a comment
There was a problem hiding this comment.
still have the inDiscoveryExperiment question, but it's not blocking.
Follow up work from #15276
conda.ts. New code added inCondaServiceis mainly taken from old code inCondaLocatorService, andCondaEnvironmentLocator.