[DYN-3343] Automatically add loaded packages as crash detail #11500
Conversation
update master
update master
This reverts commit c78dfe9.
Update master
Update master
Master update from public repo
Update master
master update
Update master
Update master
Update master
Update master
Update branch
update master
|
@Astul-Betizagasti I noticed your fork head is not clean, you may want to clean your master in the future before creating the branch on your fork |
|
@QilongTang @aparajit-pratap, Just pushed changes that should address all the previous comments, let me know what you think, thanks. |
|
All changes made, here are some gifs showing the behavior: Some loaded packages In the case that @mjkkirschner mentioned, when the package loader is null, the behavior will be similar to the first gif but it will say "(Fill in here)" instead of "No packages where found." |
|
LGTM with one last comment |
| } | ||
| else | ||
| { | ||
| markdownText = "No loaded packages where found."; |
| } | ||
| else | ||
| { | ||
| markdownText = "No loaded packages were found."; |
There was a problem hiding this comment.
So I thought about if we need to convert these into resource strings but think it may help the customer but not us. What do you guys think @DynamoDS/dynamo
| markdownText = "### Loaded Packages" + Environment.NewLine; | ||
| foreach (var name in packagesNames) | ||
| { | ||
| markdownText += "- " + name + Environment.NewLine; |
There was a problem hiding this comment.
Also do you think we can easily get package versions as well? @Astul-Betizagasti
|
|
|
@Astul-Betizagasti Can you add a unit test in |
| @@ -80,7 +85,7 @@ public void StackTraceIncludedInReport() | |||
| var dynamoVersion = AssemblyHelper.GetDynamoVersion().ToString(); | |||
|
|
|||
| // Create a crash report to submit | |||
| var crashReport = Wpf.Utilities.CrashUtilities.BuildMarkdownContent(dynamoVersion, StackTrace); | |||
| var crashReport = Wpf.Utilities.CrashUtilities.BuildMarkdownContent(dynamoVersion, Packages); | |||
There was a problem hiding this comment.
I would assert below that decoded contains package info. And also make a new test that covers the default state of the API
There was a problem hiding this comment.
Did the change suggested on the test, but can you expand on the other test you mentioned? The API should work the same as before the changes with the exception of it including more information (which is being checked on that test you linked with the changes you suggested). Are you referring to the method PackagesToMakrdown(PackageLoader packageLoader) in CrashUtilities.cs?
There was a problem hiding this comment.
yes. I would like a test covering the default state so we can assert the API defaults to No loaded packages were found.
|
Thanks @Astul-Betizagasti Looks good. Once the unit test updated, we should be able to wrap this one up |
Purpose
Automatically include loaded packages information when creating a crash report as a Github Issue through Dynamo
Declarations
Check these if you believe they are true
*.resxfilesReviewers
Aaron Tang (@QilongTang )
FYIs
Alfredo Pozo (@alfredo-pozo )