Skip to content
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

Add test if not yet present in dictionary. #2000

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Apr 5, 2024

@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 #1997

@farlee2121
Copy link
Contributor

farlee2121 commented Apr 5, 2024

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
Copy link
Contributor Author

nojaf commented Apr 6, 2024

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
Copy link
Contributor

farlee2121 commented Apr 7, 2024

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
Copy link
Contributor Author

nojaf commented Apr 8, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

nojaf commented Apr 9, 2024

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
Copy link
Contributor

farlee2121 commented Apr 9, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

nojaf commented Apr 11, 2024

Thanks @farlee2121! I appreciate the collaboration here!

@farlee2121
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New tests not detected
2 participants