Add external access to stacktrace APIs for Unit Testing #58082

ryzngard · 2021-12-02T21:08:07Z

Adds an API for single line use with navigation either based on symbol (TryFindMethodDefinitionAsync) or document name/line number (if available)

shyamnamboodiripad · 2021-12-04T01:12:14Z

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

+
+            if (result.ParsedFrames.Length > 1)
+            {
+                throw new InvalidOperationException();


Why not expose an API to parse an entire stack trace and return IEnumerable<Frame> (in addition to the current per-frame API)?

I haven't looked into exactly how we will consume this on UT side. But for completeness it would be nice to have an API that supports parsing an entire trace (especially since the internal API supports this).

Whatever works best for the UT team. We can do either. This was my guess at "best" way to do it so you handle line split and we parse a single line. If you want the whole trace to be handled by us then IEnumerable makes sense

Its not a huge issue but I think handling the whole trace in here may be better since that doesn't preclude passing in individual lines.

In other words, with an API that returns IEnumerable, I can still pass in individual lines and call .Single on the retuned enumerable on our side. However, the opposite is not possible (i.e. I can never pass in an entire stack trace if the API only supports passing one at a time).

shyamnamboodiripad · 2021-12-04T01:16:03Z

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

+                throw new InvalidOperationException();
+            }
+
+            return new Frame(result.ParsedFrames.Single(), _stackTraceService);


nit: Since this is an array can we say result.ParsedFrames[0]?

Depends on #58082 (comment)

If we do one result, then does multiple make sense or is that an error?

Yes if we support only a single API here that returns multiple this is a moot point.

shyamnamboodiripad · 2021-12-04T01:33:31Z

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

+    {
+        private readonly IStackTraceExplorerService _stackTraceService;
+
+        public UnitTestingStackTraceService(HostWorkspaceServices services)


Can this be changed to accept Workspace instead? This can then be used to access Workspace.Services here. The Workspace can also be threaded down to Frame and Definition below to avoid the need for caller to pass it when calling Definition.TryNavigateToAsync below.

(You could probably move the call to Workspace.Services to happen inside the Frame constructor.)

Sure, this was based off another item. Whatever is easiest we can accomodate

shyamnamboodiripad · 2021-12-04T01:35:03Z

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

+            private readonly ParsedFrame _parsedFrame;
+            private readonly IStackTraceExplorerService _stackTraceService;
+
+            public Frame(ParsedFrame parsedFrame, IStackTraceExplorerService service)


Should this constructor be internal instead?

hmm... probably :)

Ah I was thinking that the constructor needn't be exposed to UT. But I guess that is not possible since this is not a nested struct. So its ok to ignore this (and the below) comment.

Actually wait :) this is a nested struct - so it would be nice to make this private since nothing else needs to construct it :) @ryzngard

They can't be private because then the API couldn't construct them. The only other option is to make an interface and have the implementing type private. Not sure it's worth the trouble

shyamnamboodiripad

LGTM modulo comments 👍🏾 Thanks much for exposing the APIs!!

shyamnamboodiripad · 2021-12-04T01:35:13Z

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

+        {
+            private readonly FindUsages.DefinitionItem _definition;
+
+            public Definition(FindUsages.DefinitionItem definition)


Should this constructor be internal instead?

vritant24

LGTM

shyamnamboodiripad · 2021-12-06T21:38:47Z

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

+                _definition = definition;
+            }
+
+            public async Task<bool> TryNavigateToAsync(Workspace workspace, bool showInPreviewTab, bool activateTab, CancellationToken cancellationToken)


Can we eliminate the Workspace parameter here since it is already passed in the constructor to UnitTestingStackTraceService above?

Seems reasonable. As a rule I don't like holding onto data for people that might change, but I think workspace reference is safe here.

shyamnamboodiripad · 2021-12-06T21:41:26Z

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

+        private readonly IStackTraceExplorerService _stackTraceService;
+
+        public UnitTestingStackTraceService(Workspace workspace)
+            => _stackTraceService = workspace.Services.GetRequiredService<IStackTraceExplorerService>();


consider: Store the Workspace in the field and pass it down to the Frame constructor below. Then you could only fetch stackTraceService inside Frame (since its only used inside that struct).

CyrusNajmabadi · 2021-12-07T01:46:37Z

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

+            private readonly IStackTraceExplorerService _stackTraceService;
+            private readonly Workspace _workspace;
+
+            public Frame(ParsedFrame parsedFrame, IStackTraceExplorerService service, Workspace workspace)


make this internal.

If the enclosing type is internal does that really matter?

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

ryzngard · 2022-01-05T00:39:37Z

@sharwell thanks for the detailed review. I think I followed the correct pattern this time. PTAL

ryzngard · 2022-01-12T22:08:19Z

@sharwell PTAL

sharwell

Mostly correct, just two types need to move to Api namespace

src/Features/Core/Portable/ExternalAccess/UnitTesting/UnitTestingDefinitionItemWrapper.cs

src/Features/Core/Portable/ExternalAccess/UnitTesting/UnitTestingParsedFrameWrapper.cs

ryzngard · 2022-01-21T22:19:10Z

Ping @sharwell

Changes were made that were requested. If there's anything else please let me know and I'll do a follow up PR

ryzngard added 3 commits December 2, 2021 13:01

Add external access to stacktrace APIs for Unit Testing

f4c0c0f

Switch to a single line parsing and frame

4fbc6c4

Handle cases where no frames are parsed

eca50ad

ryzngard requested review from shyamnamboodiripad and vritant24 December 2, 2021 21:08

ryzngard requested a review from a team as a code owner December 2, 2021 21:08

dotnet-issue-labeler bot added the Area-IDE label Dec 2, 2021

ryzngard requested a review from sharwell December 2, 2021 21:08

JoeRobich approved these changes Dec 2, 2021

View reviewed changes

davidwengier approved these changes Dec 2, 2021

View reviewed changes

runfoapp bot mentioned this pull request Dec 2, 2021

Microsoft.CodeAnalysis.CSharp.CommandLine.UnitTests.CommandLineTests.ArgumentParsing is flaky #58077

Open

shyamnamboodiripad reviewed Dec 4, 2021

View reviewed changes

shyamnamboodiripad approved these changes Dec 4, 2021

View reviewed changes

ryzngard added 2 commits December 4, 2021 01:38

Change to return all frames instead of single

89b5683

Use Workspace in constructor

d1a1083

ryzngard requested a review from shyamnamboodiripad December 4, 2021 09:39

vritant24 approved these changes Dec 6, 2021

View reviewed changes

shyamnamboodiripad reviewed Dec 6, 2021

View reviewed changes

shyamnamboodiripad approved these changes Dec 6, 2021

View reviewed changes

Update to hold onto workspace

0de304c

CyrusNajmabadi reviewed Dec 7, 2021

View reviewed changes

src/Features/Core/Portable/ExternalAccess/UnitTesting/API/UnitTestingStackTraceService.cs Outdated Show resolved Hide resolved

sharwell requested changes Dec 7, 2021

View reviewed changes

runfoapp bot mentioned this pull request Dec 7, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

ryzngard added 2 commits January 4, 2022 15:32

Merge branch 'main' into features/unit_testing_stacktrace_api

0b13b6c

Use interface accessor and factory

8034b51

ryzngard requested a review from sharwell January 5, 2022 00:38

sharwell previously requested changes Jan 14, 2022

View reviewed changes

src/Features/Core/Portable/ExternalAccess/UnitTesting/UnitTestingDefinitionItemWrapper.cs Outdated Show resolved Hide resolved

src/Features/Core/Portable/ExternalAccess/UnitTesting/UnitTestingParsedFrameWrapper.cs Outdated Show resolved Hide resolved

Move Api exposed wrappers to Api namespace

ddafc8a

ryzngard requested a review from sharwell January 19, 2022 00:57

ryzngard enabled auto-merge (squash) January 20, 2022 01:43

ryzngard merged commit c805b93 into dotnet:main Jan 27, 2022

ghost added this to the Next milestone Jan 27, 2022

ryzngard deleted the features/unit_testing_stacktrace_api branch January 27, 2022 18:14

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external access to stacktrace APIs for Unit Testing #58082

Add external access to stacktrace APIs for Unit Testing #58082

ryzngard commented Dec 2, 2021 •

edited

Loading

shyamnamboodiripad Dec 4, 2021

ryzngard Dec 4, 2021

shyamnamboodiripad Dec 4, 2021

shyamnamboodiripad Dec 4, 2021

ryzngard Dec 4, 2021

shyamnamboodiripad Dec 4, 2021

shyamnamboodiripad Dec 4, 2021 •

edited

Loading

ryzngard Dec 4, 2021

shyamnamboodiripad Dec 4, 2021 •

edited

Loading

ryzngard Dec 4, 2021

shyamnamboodiripad Dec 4, 2021 •

edited

Loading

shyamnamboodiripad Dec 6, 2021

ryzngard Dec 7, 2021

shyamnamboodiripad left a comment

shyamnamboodiripad Dec 4, 2021

vritant24 left a comment

shyamnamboodiripad Dec 6, 2021

ryzngard Dec 6, 2021

shyamnamboodiripad Dec 6, 2021

CyrusNajmabadi Dec 7, 2021

ryzngard Dec 7, 2021

ryzngard commented Jan 5, 2022

ryzngard commented Jan 12, 2022

sharwell left a comment

ryzngard commented Jan 21, 2022

Add external access to stacktrace APIs for Unit Testing #58082

Add external access to stacktrace APIs for Unit Testing #58082

Conversation

ryzngard commented Dec 2, 2021 • 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

shyamnamboodiripad Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Dec 4, 2021 • edited Loading

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

Choose a reason for hiding this comment

vritant24 left a 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

ryzngard commented Jan 5, 2022

ryzngard commented Jan 12, 2022

sharwell left a comment

Choose a reason for hiding this comment

ryzngard commented Jan 21, 2022

ryzngard commented Dec 2, 2021 •

edited

Loading

shyamnamboodiripad Dec 4, 2021 •

edited

Loading

shyamnamboodiripad Dec 4, 2021 •

edited

Loading

shyamnamboodiripad Dec 4, 2021 •

edited

Loading