Stop running cohosting tests with and without FUSE #11592
Conversation
These could arguably still be "wrong" in Fuse, but since they're stable we may as well have the coverage.
More to come in a follow up here, that also changes product code
…ohost doesn't vary
Cohosting is always FUSE, so never has design time documents. Language Server can have both, and the formatting engine decides based on interface implementation, not preproccesor directive.
Previous behaviour seemed like the old formatting engine being over-zealous. This change makes both engines behave the same, which I prefer to skipping
Counter and SurveyPrompt were in the json, but aren't part of the framework
Now that we're using the real framework, this test is valid in the language server
These tests had hard coded positions in them, for locations in the generated C# file. This is very fragile, and broke with other changes. Two of these scenarios are covered by other unit tests, and one by an integration test.
|
Since nobody has looked at this yet, I've pushed a few more commits and done the formatting test changes too. Commit-at-a-time is recommended for those, for sure. The TL;DR is that it used to be the language server couldn't run FUSE due to test infra gaps, so cohosting tests did the heavy lifting there. With cohosting now exclusively FUSE, that didn't make sense, so I made cohosting exclusively new formatting engine, and upgraded the language server test infra so it can do FUSE now. |
There was a problem hiding this comment.
Looks good! I also recommend removing these comments from Directory.Build.props now that FORMAT_FUSE is gone.
Lines 51 to 52 in 2798396
Also, please update your PR description. It specifically says that you weren't going to make the changes in this PR that you went ahead and did. I'd hate for somebody in the distant future to look at this commit history and think poorly of you.
| var project = workspace.CurrentSolution.AddProject(projectInfo).GetProject(projectId); | ||
|
|
||
| var compilation = await project.GetCompilationAsync(cancellationToken).ConfigureAwait(false); | ||
| if (compilation is null) |
There was a problem hiding this comment.
I've seen several places in Razor where we check the result of Project.GetCompilationAsync for null, but I don't think that can ever happen unless it's a non-C# or VB project. IIRC, Project.GetCompilatoinAsync can only return null if Project.SupportsCompilation returns false. For test scenarios, it might be worth just doing Assert.NotNull(compilation) to highlight that the failure would be unexpected.
There was a problem hiding this comment.
Turns out this whole compilation wasn't ever used 🤦♂️
.../test/Microsoft.VisualStudio.LanguageServices.Razor.Test/TestFiles/SemanticTokens/Legacy.txt
Show resolved
Hide resolved
| await VerifySemanticTokensAsync(input, colorBackground, precise); | ||
| } | ||
|
|
||
| [FuseTheory(Skip = "https://github.com/dotnet/razor/issues/10857 and https://github.com/dotnet/razor/issues/11329")] |
There was a problem hiding this comment.
I'd recommend either leaving the theory skipped with the linked issues or dropping a note in both issues that you've updated the test. Both issues are still open. Your changes here might allow @chsienki to verify and close out #10857. And you noted in #11329 that there's a test referencing that issue but you're about to remove the reference.
There was a problem hiding this comment.
Good catch, will update the issues.
ugh, thank you! I did a Find in Files in VS and it got no results, I should have thought to do one in VS Code too :) |
[View Complete Diff of Changes](https://github.com/dotnet/razor/compare/2798396c3481573aa49f9c792179ebbb5e183dca...c0bd75d99369adcd5a2f7e1f1ac42bee8a3bf619?w=1) * Move VS Code To Pull Diagnostics (#11602) (PR: [#11602](dotnet/razor#11602)) * Upgrade to net9 (#11535) (PR: [#11535](dotnet/razor#11535)) * Cleanup CompletionTriggerAndCommitCharacters (#11600) (PR: [#11600](dotnet/razor#11600)) * Add constraints to CaptureParameters method (#11530) (PR: [#11530](dotnet/razor#11530)) * [main] Update dependencies from dotnet/source-build-reference-packages (#11598) (PR: [#11598](dotnet/razor#11598)) * [FUSE] Layout mapping (#11567) (PR: [#11567](dotnet/razor#11567)) * Stop running cohosting tests with and without FUSE (#11592) (PR: [#11592](dotnet/razor#11592))
Now that the source generator is in use, we don't ever actually use non-FUSE in cohosting, so we were just running all our tests twice for no reason. This fixes that.
There is more to do to follow up here for formatting tests, and removing theDid it.FORMAT_FUSEpreprocessor directive, but that will affect shipping product code so I'll do it serparately.