Remove portions of SolutionCrawler not used by UnitTesting team. #64330

CyrusNajmabadi · 2022-09-27T23:00:33Z

Should be removed commit by commit.

…diabledUnusedFunctionality

CyrusNajmabadi · 2022-09-28T15:20:11Z

...es/Core/Portable/ExternalAccess/UnitTesting/API/NewUnitTestingIncrementalAnalyzerProvider.cs

            highPriorityForActiveFile: false,
+#endif


unit testing always passes 'false' for this value. so no point in having it.

CyrusNajmabadi · 2022-09-28T15:20:44Z

src/Features/Core/Portable/ExternalAccess/UnitTesting/API/NewUnitTestingIncrementalAnalyzer.cs

@@ -25,6 +25,7 @@ public Task AnalyzeDocumentAsync(Document document, SyntaxNode bodyOpt, UnitTest
        public Task AnalyzeProjectAsync(Project project, bool semanticsChanged, UnitTestingInvocationReasons reasons, CancellationToken cancellationToken)
            => _implementation.AnalyzeProjectAsync(project, semanticsChanged, reasons, cancellationToken);

+#if false // Not used in unit testing crawling


all the methods that unit testing just returns Task.CompletedTask are not necessary to support. so each commit ifdefs one out and works backwards, removing anything uneeded.

CyrusNajmabadi · 2022-09-28T15:21:05Z

.../Core/Portable/ExternalAccess/UnitTesting/SolutionCrawler/IUnitTestingIncrementalAnalyzer.cs

@@ -23,10 +25,14 @@ internal interface IUnitTestingIncrementalAnalyzer
        Task DocumentResetAsync(Document document, CancellationToken cancellationToken);

        Task AnalyzeSyntaxAsync(Document document, UnitTestingInvocationReasons reasons, CancellationToken cancellationToken);
+#endif
+
        Task AnalyzeDocumentAsync(Document document, SyntaxNode bodyOpt, UnitTestingInvocationReasons reasons, CancellationToken cancellationToken);
        Task AnalyzeProjectAsync(Project project, bool semanticsChanged, UnitTestingInvocationReasons reasons, CancellationToken cancellationToken);

        Task RemoveDocumentAsync(DocumentId documentId, CancellationToken cancellationToken);


these three methods are the only thing unit testing actually uses.

CyrusNajmabadi · 2022-09-28T15:21:48Z

...Core/Portable/ExternalAccess/UnitTesting/SolutionCrawler/UnitTestingSolutionCrawlerLogger.cs

            foreach (var analyzer in analyzers)
            {
                analyzer.LogAnalyzerCountSummary();
            }
+#endif


an example of working backwards. becausse unit testing doesn't use LogAnalyzerCountSummary, we ifdefed it out. so this loop here is also unnecessary.

CyrusNajmabadi · 2022-09-28T15:22:07Z

.../ExternalAccess/UnitTesting/SolutionCrawler/UnitTestingSolutionCrawlerRegistrationService.cs

                        initializeLazily: true,
+#endif


as this is always passed 'true' in, it's not needed as an actual customization flag and has been removed.

CyrusNajmabadi · 2022-09-28T15:23:19Z

...esting/SolutionCrawler/UnitTestingWorkCoordinator.UnitTestingIncrementalAnalyzerProcessor.cs

                private readonly UnitTestingHighPriorityProcessor _highPriorityProcessor;
+#endif


no need for a high-pri-processor at all as unit testing says that its analyzer doesn't get high pri processing privileges on any event. (i still need to see if we need the low pri one).

CyrusNajmabadi · 2022-09-28T15:23:43Z

...esting/SolutionCrawler/UnitTestingWorkCoordinator.UnitTestingIncrementalAnalyzerProcessor.cs

                    if (!initializeLazily)
                    {
                        // realize all analyzer right away
                        _ = lazyActiveFileAnalyzers.Value;
                        _ = lazyAllAnalyzers.Value;
                    }
+#endif


since initializeLazily is always true, no need to do this realization work.

CyrusNajmabadi · 2022-09-28T15:28:31Z

...UnitTesting/SolutionCrawler/UnitTestingWorkCoordinator.UnitTestingNormalPriorityProcessor.cs

@@ -352,6 +361,7 @@ private async Task ProcessDocumentAsync(ImmutableArray<IUnitTestingIncrementalAn
                                        await ProcessOpenDocumentIfNeededAsync(analyzers, workItem, textDocument, isOpen, cancellationToken).ConfigureAwait(false);
                                        await ProcessCloseDocumentIfNeededAsync(analyzers, workItem, textDocument, isOpen, cancellationToken).ConfigureAwait(false);
                                    }
+#endif


note: as unit testing doesn't care abotu open/close events, the next pr removes those events from teh ones that VS will send to OOP.

I'm curious whether this would have triggered analysis that would have called AnalyzeDocument() on the UT analyzer when files are opened (and whether commenting it out can cause any change in semantics esp. if we want to keep the highPriorityForActiveFiles flag above). I am assuming not - but the 'if needed' in the name of the above API prompted this question...

(For example, would tests from a document that was opened have been discovered faster because of the above calls? Would commenting it out mean that opening a document no longer expediates discovery of contained tests. The example is hypothetical - but wanted to check if we would see any subtle changes in semantic like that by commenting this out...)

so all ProcessOpenDocumentIfNeededAsync does it run the DocumentOpenAsync method on all registered analyzers. so for UnitTesting that was a no-op. So these two calls were no ops. so i if'def'ed it out. :)

shyamnamboodiripad · 2022-09-28T17:15:51Z

...es/Core/Portable/ExternalAccess/UnitTesting/API/NewUnitTestingIncrementalAnalyzerProvider.cs

            highPriorityForActiveFile: false,
+#endif


I am surprised by this. My assumption was that the crawler always prioritized analysis in active files (and was not aware that analyzers need to opt-in for this). I wonder if we are leaving some responsiveness on the table (for test discovery) by setting this to false.

Is there any harm in preserving this so that we could experiment with switching this to true down the line?

i'm preservinb this by keeping the code in teh ifdef around. when you guys take over this, you can totally try flipping the switch on this. but i'm not a fan of speculatively keeping this around with all the complexity when it's not even being used currently, and all this code is going to be 'legacy' and ideally replaced with something better asap :)

Fair enough. So long as the code is available, we can experiment with it.

The only reason I was asking is that this could be an easy perf win that users would notice (especially when adding tests within larger solutions). It would be faster to experiment and improve the user experience with the existing code than to say that we need to first move over to a pull-based model before we can do that :)

i think that's totally fine :) that's why i'm leaving the code in there. you can def add/augment it going forward to try things out. my hope though is that we can start with an extremely small and lightweight kernel that is easy to maintain versus having to pull in everything in the crawler space.

shyamnamboodiripad · 2022-09-28T17:18:49Z

src/Features/Core/Portable/ExternalAccess/UnitTesting/API/NewUnitTestingIncrementalAnalyzer.cs

@@ -84,5 +87,7 @@ public void LogAnalyzerCountSummary()
        public void Shutdown()


The nested #if false above can probably be removed.

shyamnamboodiripad · 2022-09-28T17:23:54Z

...s/UnitTesting/SolutionCrawler/UnitTestingWorkCoordinator.UnitTestingHighPriorityProcessor.cs

@@ -95,11 +96,13 @@ public void Enqueue(UnitTestingWorkItem item)
                            return;
                        }

+#if false // Not used in unit testing crawling


Looks like this is nested inside another #if false and can potentially be omitted

shyamnamboodiripad

👍🏾

CyrusNajmabadi and others added 11 commits September 21, 2022 15:27

Disable unused functionality

3a9f2a2

Disable unused functionality

b87d6a5

Disable unused functionality

46b5344

Disable unused functionality

ae0d333

Disable unused functionality

1b8f7f1

Disable unused functionality

8949601

Remove priority concept

6c0f7ce

Merge branch 'newUnitTestingApis' into diabledUnusedFunctionality

e68b3b3

Merge branch 'newUnitTestingApis' into diabledUnusedFunctionality

2cc4c64

Disable unused functionality

6578dc0

Merge remote-tracking branch 'upstream/features/solutionEvents' into …

524ac0c

…diabledUnusedFunctionality

CyrusNajmabadi requested review from a team as code owners September 27, 2022 23:00

dotnet-issue-labeler bot added the Area-IDE label Sep 27, 2022

CyrusNajmabadi changed the base branch from main to features/solutionEvents September 27, 2022 23:00

CyrusNajmabadi added 3 commits September 27, 2022 16:27

Disable unused functionality

939e249

Make synchronous

1542b2f

Disable unused functionality

8b88033

runfoapp bot mentioned this pull request Sep 28, 2022

FixAllOccurrencesIgnoresGeneratedCode is flaky #56625

Open

CyrusNajmabadi requested review from tmat and shyamnamboodiripad September 28, 2022 15:19

CyrusNajmabadi commented Sep 28, 2022

View reviewed changes

proper pattern

89840f1

CyrusNajmabadi commented Sep 28, 2022

View reviewed changes

CyrusNajmabadi mentioned this pull request Sep 28, 2022

Remove 'file open/close' eventing from the new unit-testing solutioncrawler #64339

Merged

shyamnamboodiripad reviewed Sep 28, 2022

View reviewed changes

remove nested ifdefs

6dc2ec5

CyrusNajmabadi requested a review from shyamnamboodiripad September 28, 2022 18:12

CyrusNajmabadi enabled auto-merge September 28, 2022 18:17

shyamnamboodiripad approved these changes Sep 28, 2022

View reviewed changes

CyrusNajmabadi merged commit 6e189ab into dotnet:features/solutionEvents Sep 28, 2022

CyrusNajmabadi deleted the diabledUnusedFunctionality branch September 28, 2022 19:04

allisonchou mentioned this pull request Oct 11, 2022

[Automated] PRs inserted in VS build main-33011.173 #64652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove portions of SolutionCrawler not used by UnitTesting team. #64330

Remove portions of SolutionCrawler not used by UnitTesting team. #64330

CyrusNajmabadi commented Sep 27, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

shyamnamboodiripad Sep 28, 2022 •

edited

Loading

CyrusNajmabadi Sep 28, 2022

shyamnamboodiripad Sep 28, 2022 •

edited

Loading

CyrusNajmabadi Sep 28, 2022

shyamnamboodiripad Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

shyamnamboodiripad Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

shyamnamboodiripad Sep 28, 2022

CyrusNajmabadi Sep 28, 2022

shyamnamboodiripad left a comment

		private readonly UnitTestingHighPriorityProcessor _highPriorityProcessor;
		#endif

		@@ -84,5 +87,7 @@ public void LogAnalyzerCountSummary()
		public void Shutdown()

Remove portions of SolutionCrawler not used by UnitTesting team. #64330

Remove portions of SolutionCrawler not used by UnitTesting team. #64330

Conversation

CyrusNajmabadi commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad left a comment

Choose a reason for hiding this comment

shyamnamboodiripad Sep 28, 2022 •

edited

Loading

shyamnamboodiripad Sep 28, 2022 •

edited

Loading