search

ionide / ionide-vscode-fsharp

VS Code plugin for F# development
http://ionide.io
MIT License
849 stars 276 forks source link

Add test if not yet present in dictionary. #2000

Open nojaf opened 2 months ago

nojaf commented 2 months ago

@farlee2121 I could be wrong but this seemed to help to show the new tests in my project.

Reasoning: the newly added test won't be in the existing dictionary and the check needs to be negated. I could be wrong, please let me know if this makes sense.

Fixes https://github.com/ionide/ionide-vscode-fsharp/issues/1997

farlee2121 commented 2 months ago

If I remember right, that condition is adding tests to the explorer when we've previously been able to match a non-standard location (e.g. a referenced, but not directly nested testList in Expecto). The displaced fragment cache is constructed beforehand and isn't being modified in mergeCodeUpdates.

I ran some testing and something is definitely off about the explore behavior on code update. Renaming methods is causing them to disappear. I am able to add and remove test methods and see live updates though.

nojaf commented 2 months ago

Similarly, when I set "FSharp.TestExplorer.AutoDiscoverTestsOnLoad" to false, my tests are no longer discovered at all. Even though I see the server sending the notifications with the correct information.

The reason I want FSharp.TestExplorer.AutoDiscoverTestsOnLoad to be false is that it runs all unit tests when I open my solution. I get why this is useful in some cases but for the Fantomas tests, all tests can be discovered via the AST quite accurately.

farlee2121 commented 2 months ago

If I'm understanding right, you want to discover tests on load via code analysis alone and not running tests.

That works against the base assumptions of the test explorer. A lot of the work I did was to re-orient the test explorer around test results since the code analysis results were causing flakiness (i.e. incorrectly located tests which broke the explorer).

Tests can still be discovered when AutoDiscoverTestsOnLoad is false by refreshing the explorer manually, which will run tests.

nojaf commented 2 months ago

If I'm understanding right, you want to discover tests on load via code analysis alone and not running tests.

Yes, this used to work fine for me when I originally ported the test explorer code from Neptune. Right now I'm a bit annoyed that all my tests are needlessly run when I open vscode (with setting on) or when I use the refresh action (with setting off).

The notifications work fine for me, this should also be possible to work with.

farlee2121 commented 2 months ago

My concern is that building from code analysis results would re-introduce the issues outlined in #1874. Test results aren't always the fastest, but they are a consistently reliable method for constructing the test explorer.

I'm open to suggestions.

nojaf commented 2 months ago

I would go for some middle ground here. Not all projects face the issue of #1874, so there should be an opt-out. In some codebases, it really is that simple that every [<Test>] equals a runnable test entry.

My suggestion would be to always process the incoming notifications. This seems to be off when "FSharp.TestExplorer.AutoDiscoverTestsOnLoad": false I suspect. And I would assume the server sends them anyway.

farlee2121 commented 2 months ago

Building the test explorer from code analysis isn't a matter of leaving it on when result-based updates are off, it'd require a separate mode. Currently, code analysis is not used as a primary truth source. It relies on the baseline provided by test results to prevent the unusable states described in #1874 (i.e. where run all doesn't work anymore).

Though, the explorer now should be able to run when non-existent tests are selected and be refreshed from test results if it gets off. Perhaps a "trust all code analysis" mode wouldn't get the explorer into an unrecoverable state.

farlee2121 commented 2 months ago

I've been digging a bit to understand how Visual Studio manages test location discovery. I'm still getting a handle on it, but they seem to manage the whole discovery process through the Adapter Extensibility model.

Maybe that's the way forward here too? Then we're capable of anything VS is capable of and we can offload discovery to the adapters and hopefully some of the orchestration to vstest.

I'll keep digging.

nojaf commented 2 months ago

Thanks @farlee2121! I appreciate the collaboration here!

farlee2121 commented 2 months ago

I've not gotten as much research into this as I'd hoped, but I did poke around VS Test and found that this is the package meant to smooth integration for editors: Microsot.TestPlatform.TranslationLayer. They have an example and a playground for internal developers that demonstrate it's use.