Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add external access to stacktrace APIs for Unit Testing #58082
Add external access to stacktrace APIs for Unit Testing #58082
Changes from 3 commits
f4c0c0f
4fbc6c4
eca50ad
89b5683
d1a1083
0de304c
0b13b6c
8034b51
ddafc8a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, this was based off another item. Whatever is easiest we can accomodate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since this is an array can we say
result.ParsedFrames[0]
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on #58082 (comment)
If we do one result, then does multiple make sense or is that an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes if we support only a single API here that returns multiple this is a moot point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this constructor be internal instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... probably :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually wait :) this is a nested struct - so it would be nice to make this private since nothing else needs to construct it :) @ryzngard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this constructor be internal instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we eliminate the Workspace parameter here since it is already passed in the constructor to UnitTestingStackTraceService above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.