Hook up the new remote-solution-events-stream to the UnitTest-SolutionCrawler. #64150
Conversation
…unitTestingRegistration
CyrusNajmabadi
changed the base branch from
main
to
features/solutionEvents
| #if false // Not used in unit testing crawling | ||
| Workspace workspace | ||
| #endif | ||
| ); |
There was a problem hiding this comment.
Have validated that unittesting's incremental analyzer does not use this: http://index/?rightProject=Microsoft.CodeAnalysis.Features&file=ExternalAccess%5cUnitTesting%5cAPI%5cUnitTestingIncrementalAnalyzerProvider.cs&line=26
| IUnitTestingIncrementalAnalyzer? CreateIncrementalAnalyzer( | ||
| #if false // Not used in unit testing crawling | ||
| Workspace workspace | ||
| #endif |
There was a problem hiding this comment.
i have validated that unit testing does not use this workspace: http://index/?rightProject=Microsoft.CodeAnalysis.Features&file=ExternalAccess%5cUnitTesting%5cAPI%5cUnitTestingIncrementalAnalyzerProvider.cs&line=26
There was a problem hiding this comment.
This is also a general pattern i'm using in the fork. Instead of deleting, i'm IFDEF'ing out so we can see how this drifts from teh original.
There was a problem hiding this comment.
Wouldn't simple diff of the files show that? Perhaps we could keep the file names the same.
There was a problem hiding this comment.
possibly... though i worry about the diffs getting too far apart (and things like naming confusing it). I'm flexible on this htough. if we prefer just deleting, that's also ok with me.
| } | ||
|
|
||
| internal sealed class UnitTestingRegistration | ||
| { | ||
| public readonly int CorrelationId; | ||
| public readonly Workspace Workspace; | ||
| public readonly string WorkspaceKind; | ||
| public readonly SolutionServices Services; |
There was a problem hiding this comment.
the general idea here was to get us off of 'Workspace' and onto the constituent pieces that are needed instead. These pieces are possible today to get (obviously, since we still have workspaces), but they shoudl also be hopefully not a problem to get even as we heavily refactor oop and get rid of hte RemoteWorkspace type.
| ProjectId? preferableProjectId, | ||
| #if false // Not used in unit testing crawling | ||
| ProjectDependencyGraph dependencyGraph, | ||
| IDiagnosticAnalyzerService? service, |
There was a problem hiding this comment.
the dependency graph and diagservice are used by solution-crawler to order what files to analyze, preferring files with diagnostics in them (so they can be removed quickly if fixed). Such a concept is irrelevant for unittest source-discovery. So this greatly simplifies things and prevents the need for workspace access to compute this data and pass it along.
There was a problem hiding this comment.
If a change is made in one project, was the dependency graph being used to ensure that the files in this project are analyzed before other projects that depend on it?
There was a problem hiding this comment.
@shyamnamboodiripad No, we would do this:
foreach (var dependingProjectId in dependencyGraph.GetProjectsThatDirectlyDependOnThisProject(projectId))
{
if (workQueue.ContainsKey(dependingProjectId) && analyzerService?.ContainsDiagnostics(Workspace, dependingProjectId) == true)
{
return dependingProjectId;
}
}So it was only relevant if the projects actually contain diagnostics. if not, then the dependency graph had no effect.
| @@ -157,8 +161,6 @@ public void Dispose() | |||
| RaiseCancellation_NoLock(cancellations); | |||
| } | |||
|
|
|||
| protected Workspace Workspace => _workspace; | |||
There was a problem hiding this comment.
goal was to get rid of storage of the workspace (and exposing it like this). that way when this doesn't prevent us from removing RemoteWorkspace.
| @@ -259,6 +273,7 @@ protected CancellationToken GetNewCancellationToken_NoLock(object key) | |||
| return projectId; | |||
| } | |||
|
|
|||
| #if false // Not used in unit testing crawling | |||
| // prefer project that directly depends on the given project and has diagnostics as next project to | |||
| // process | |||
There was a problem hiding this comment.
irrelevant to unittesting solution crawling. source-discovery is unrelated to where diagnostics are.
|
|
||
| _registration.Workspace.WorkspaceChanged += OnWorkspaceChanged; | ||
| _registration.Workspace.TextDocumentOpened += OnTextDocumentOpened; | ||
| _registration.Workspace.TextDocumentClosed += OnTextDocumentClosed; |
There was a problem hiding this comment.
instead of listening to events by hooking up to the workspace, we will instead get the events from teh new LegacySolutionEvents stream.
| SolutionServices SolutionServices { get; } | ||
| /// <inheritdoc cref="Workspace.CurrentSolution"/> | ||
| Solution CurrentSolution { get; } | ||
| } |
There was a problem hiding this comment.
this is an abstraction that allows us to just get teh data we need from a workspace, without actually exposing the workspace itself.
Note that CurrentSolution is oogy as heck, but needed for SolutionCrawler (which commonly goes back to this). See the docs here though on why this is ok for this legacy/compat purpose: https://github.com/dotnet/roslyn/pull/64150/files#diff-3e98ede73fac6d34d191fb31d946bf9df7eca9d83b24fa662a1c4e8cd30c8e94R68
| /// In other words, we are not keeping around the latest-primary-solution-snapshot to power UnitTesting | ||
| /// solution-crawling. We are keeping it around for perf around standard OOP snapshot requests, and this | ||
| /// component can appropriately leverage that. | ||
| /// </summary> |
There was a problem hiding this comment.
@tmat The above contains my thinking on why this direction is ok. This is jsut a super unfortunate aspect of the design of solution-crawler. However, it does feel like it's something we can support even as we move off of RemoteWorkspace in OOP.
There was a problem hiding this comment.
The above does require is to effectively 'expose' that we are caching teh 'latest primary solution snapshot' in OOP (outside of hte syncing system). It's not ideal, but it's also hopefully very temporary esp. as we continue to push on unittesting to get off of solution-crawler (even their fork of it).
There was a problem hiding this comment.
Couldn't solution crawler figure what the current solution is from the solution events? Every time in-proc workspace current solution is updated we send an event to OOP which has the snapshot, right?
There was a problem hiding this comment.
I.e. the solution crawler would track what the current snapshot is instead of remote workspace.
There was a problem hiding this comment.
Couldn't solution crawler figure what the current solution is from the solution events? Every time in-proc workspace current solution is updated we send an event to OOP which has the snapshot, right?
Maybe.................................
It does seem reasonable. And i think i will change to that in the next PR. Though it def makes me nervous that this will somehow be different due to some crazy aspect of SolutionCrawler taht we don't understand :D
I'm very torn on this work between trying to keep things as absolutely close to how SolutionCrawler works today, while also wanting to make judicious changes to keep things as unhacky as possible. This feels like it's on the fence. But worthwhile enough to do.
There was a problem hiding this comment.
I.e. the solution crawler would track what the current snapshot is instead of remote workspace.
Yup. Understood. And i think taht is reasonable given that only the 'host' issues the stream of workspace events, and that happens in a serialized queue. So we always just reocrd the latest and have that be conceptually the same as the one the RemoteWorkspace is holding onto.
|
|
||
| namespace Microsoft.CodeAnalysis.LegacySolutionEvents | ||
| { | ||
| internal interface ILegacyWorkspaceDescriptor |
There was a problem hiding this comment.
Isn't WorkspaceKind available from Solution as well?
There was a problem hiding this comment.
and SolutionServices also. so perhaps this just needs CurrentSolution?
There was a problem hiding this comment.
WFM. Will rename in followup.
There was a problem hiding this comment.
Isn't WorkspaceKind available from Solution as well?
and SolutionServices also. so perhaps this just needs CurrentSolution?
True! Ok, i can simplify this even further. Will do so in next PR :)
This PR contains the changes to the new unit-testing crawler to move it off from having direct Workspace dependencies (since we won't have a workspace in the future in OOP) and to hook it up to the new legacy-solution-events stream created in the prior PR. Relevant pieces are documented to look at.