Enable TestPreviewDiagnosticTaggerInPreviewPane #24783
Conversation
|
looks good. might be fixed by this - #24512 - since it is using WpfFact |
|
@heejaechang I rewrote this to no longer rely on hacks. Please review again. 😄 |
|
@Pilchie for ask mode since it includes a change to shipping code |
|
|
||
| [ImportingConstructor] | ||
| public PreviewSolutionCrawlerRegistrationServiceFactory(IDiagnosticAnalyzerService analyzerService) | ||
| public PreviewSolutionCrawlerRegistrationServiceFactory(IDiagnosticAnalyzerService analyzerService, IAsynchronousOperationListenerProvider listenerProvider) |
There was a problem hiding this comment.
this is workspace service specific to Preview Kind. the test doesn't create PreviewKind workspace. if you want this, you should create PreviewKind workspace.
There was a problem hiding this comment.
➡️ It's already created in PreviewFactoryService.CreateChangedDocumentPreviewViewAsync.
| @@ -249,16 +237,8 @@ public void TestPreviewDiagnostic() | |||
| var rightTagger = rightProvider.CreateTagger<IErrorTag>(rightBuffer); | |||
| using (var rightDisposable = rightTagger as IDisposable) | |||
| { | |||
| // wait up to 20 seconds for diagnostics | |||
| taskSource.Task.Wait(20000); | |||
| if (!taskSource.Task.IsCompleted) | |||
There was a problem hiding this comment.
is this okay? it was waiting for analysis to actually happen. I think your approach seems good. just that TestWorkspace the test creates, you need to set the workspace kind as preview. and it needs to use its own ExportProvider otherwise, listener will get messed up since it got shared with all other tests. and nit, but use FeatureAttributes.SolutionCrawler rather than DiagnosticService.
There was a problem hiding this comment.
➡️ It's waiting for DiagnosticService because that's the feature reported by PreviewSolutionCrawlerRegistrationService. Previously it was not possible to directly wait on the task created by PreviewSolutionCrawlerRegistrationService.Service.Register, but now it is.
There was a problem hiding this comment.
yea. I got it. the CreateChangedDocumentPreviewViewAsync internally creates the PreviewWorkspace.
|
Waiting for @heejaechang to sign off again on the new set of changes. |
| // wait taggers | ||
| await listenerProvider.GetWaiter(FeatureAttribute.ErrorSquiggles).CreateWaitTask(); | ||
| // wait for diagnostics and taggers | ||
| await listenerProvider.WaitAllDispatcherOperationAndTasksAsync(FeatureAttribute.DiagnosticService, FeatureAttribute.ErrorSquiggles); |
There was a problem hiding this comment.
listener....WaitAsync accepts multiple waiter but, ones that given together is expected not have dependency each other. for DiagnosticService, SolutionCrawler, ErrorSquiggle has dependency each other. like SolutionCrawler must run and finish first, then DiagnosticService and then ErrorSquiggle. so probably better to have things in order
WaitAll...(SolutionCrawler);
WaitAl...(Diagnostic...)
..
like that
There was a problem hiding this comment.
We should fix this. The operation should complete when none of the listed items are scheduled or in progress, which even handles cycles provided termination occurs.
There was a problem hiding this comment.
@heejaechang The implementation of WaitAllAsync already behaves like I described. If anything, using a single call here should be better able to ensure the work is complete before proceeding. It will catch (and wait for) cases where FeatureAttribute.ErrorSquiggles causes a new task to start for FeatureAttribute.DiagnosticService.
There was a problem hiding this comment.
he he, you are right :)
| @@ -236,7 +224,7 @@ public void TestPreviewDiagnostic() | |||
| { | |||
| var foregroundService = workspace.GetService<IForegroundNotificationService>(); | |||
|
|
|||
| var listenerProvider = new AsynchronousOperationListenerProvider(); | |||
| var listenerProvider = workspace.ExportProvider.GetExportedValue<AsynchronousOperationListenerProvider>(); | |||
There was a problem hiding this comment.
again, to test like this, it must have its own ExportProvider rather than sharing common one with other test. ExportProvider.Create....
|
Approved pending Jenkins. |
The underlying requirement that diagnostics be waitable through FeatureAttribute.DiagnosticService was recently implemented (dotnet#24783), so these tests should be reliable again. Fixes dotnet#17393
Fixes #14444
@heejaechang Can this test be updated following the work for #24690? In other words, is there now an event that the test can listen to and know that the diagnostics are updated?