Fix codelens action for opening a dropdown on parametrized tests in windows 10

[phloose]

Fixes #8627. When the path of an actual test file (given by vscode.Uri.fsPath) is later compared against the fullPath of a single item of tests.TestFiles the drive letter mismatches and aborts further actions despite the rest of the path is the same.

• 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-io]

Codecov Report

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

@@           Coverage Diff            @@
##             master   #8647   +/-   ##
========================================
  Coverage          ?   57.1%           
========================================
  Files             ?     550           
  Lines             ?   28841           
  Branches          ?    4358           
========================================
  Hits              ?   16469           
  Misses            ?   11471           
  Partials          ?     901

Impacted Files	Coverage Δ
src/client/testing/display/picker.ts	46.3% <100%> (ø)

---

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 c9d6887...9acd9b0. Read the comment docs.

[ericsnowcurrently]

This is just missing a test. Otherwise LGTM. Thanks for working on this!

[phloose]

This is just missing a test.

@ericsnowcurrently I added a test. I am not really happy with all that mocking, though. But it ensures that respective parts of the method TestDisplay.displayFunctionTestPickerUI, that are responsible for showing the picker, can be reached when the drive letters for fileName and item.fullPath are different.

Should i squash the two commits into one when everything is ok from your side?

[ericsnowcurrently]

Don't worry about squashing. We always squash-and-merge for PRs. :)

[ericsnowcurrently]

First of all, I apologize for the state of the existing tests (in that file). They are too dense and not particularly careful. So they aren't a great example, unfortunately.

Furthermore, there is basically zero coverage of the TestDisplay class's methods. Basically, there should have been a bunch of tests there already covering all the behavior and all you would have to do is add one test (based on an existing one) that exercises the relevant behavior on Windows. Obviously no such tests exist.

I spent some time yesterday looking into the possibility of (me) writing those missing tests, but doing so is going to take a lot of work. This relates in part to your earlier statement (assuming I understood you correctly):

I am not really happy with all that mocking, though.

Yeah, all the mocks aren't ideal and we are working on a better approach. Also, mocks like these are mostly the province of unit tests, as opposed to other kinds of tests. So perhaps you mean you'd rather have functional tests (running against actual system resources)?

For unit tests we want:

• low-level focus
• determinism (even on different host OS)
• explicit control of all inputs (direct & indirect)
• verification of all outputs (direct & indirect), at least relative to the suite of tests in aggregate (and usually on a per-test basis)

Consequent to all the above, I've made a number suggestions that will help align your tests with the what should have been there already. :) There are a few more comments I considered leaving, but let's start with what I have done already.

Thanks!

[phloose]

@ericsnowcurrently Thanks a lot for your very detailed review! I really appreciate that! I'll go through your suggestions and advices and change the respective parts.

Regarding the coverage i was really puzzled when i saw that there were no tests at all for the TestDisplay class... Thats the reason i setup the respective suite in general but also wasn't completly sure how to structure it because i didn't now if you would adapt to it or what else should be in there. So i am happy to lay a foundation for future tests of that class.

So perhaps you mean you'd rather have functional tests

I always prefer to have some kind of real/unmocked functionality in tests i write (be it unit or functional tests...). Since i am developing most of the time in python where mocking is really easy and flexible i am always careful when i need to mock. Sometimes i have the feeling that in the end you test that you configured your mocks right, which possibly masks other errors that bubble up later. Of course i know that especially in unit tests you need to mock when the surrounding depeneds a lot on the state of other classes/objects (like in this case with TestDisplay).

[ericsnowcurrently]

Looking good. Thanks for making those changes. I only have a few other comments.

[phloose]

@ericsnowcurrently I changed the tests according to your suggestions. In order to keep the git history clean i would prefer to squash the testing related commits into one as there are five commits that have a lot of changes in between.

[phloose]

i just saw that something changed with the FileSystem class because compiling fails?

EDIT:
Did a rebase on master and saw that FileSystem needs now PlatformService() as argument when making an instance.

[ericsnowcurrently]

Don't worry about squashing. We only squash-and-merge when landing PRs, so that will be done automatically.

[ericsnowcurrently]

This looks great. Thanks for all those changes. I'm happy with where this PR is at.

I've made a number of comments about formatting and one about maybe changing a declaration from ITestDisplay to TestDisplay. None of them are critical and I'm approving the PR.

I'll give you time to make any of those recommended changes that you want and then merge it. FYI, I'm also going to run the PR through our full CI test suite just to verify the change more broadly. However, there's nothing you need to do relative to that.

[ericsnowcurrently]

Again, thanks for all your work on this. I really appreciate your willingness to accommodate my various suggestions. 😄

[phloose]

Again, thanks for all your work on this. I really appreciate your willingness to accommodate my various suggestions. 😄

Thank you for your help and suggestions! I found that super helpful and really appreciate that! You explained everything and it made sense to me. This has definitely taken me further in terms of writing tests in general and in learning typescript!