5909 pre-fill Github issue reports with crash details #9150

radumg · 2018-10-05T21:17:45Z

Purpose

Fixes #5909 by pre-filling the new Github issue page with

an appropriate title that includes Dynamo version number : Crash report from Dynamo 2.0.1
issue body filled with the crash details text, displayed in the CrashPrompt window bottom text box

Currently pressing Submit to Github takes you here :

With the new behaviour, pressing Submit to Github takes you here :

In the above image, the This is the error text body would be replaced by the actual details of the crash stack trace, as per original screenshot in #5909 .

I can't seem to be able to make Dynamo crash on purpose right now (funny how that works...), so dunno how to replicate.

Other notes

sends users to https://github.com/DynamoDS/Dynamo/issues/new by default when no crash text is supplied (friendlier than sending to the super-busy all issues page)
as per current implementation, you still need to be logged in to Github to be able to submit the issue

Further improvements

We could add the rest of the current issue template text to the body and wrap the crash dump inside a nice code block, but I'm afraid that'd make the query string way too long. Open to ideas/suggestions ?

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@Racel

FYIs

@andydandy74 , @johnpierson

you want to make it easy for users to report the issue, not take them to a page where they stare at a wall of 600 unsolved issues and having to figure out HOW to open/submit a new one

mjkkirschner · 2018-10-05T21:35:58Z

This is great, we were just talking about this -additionally would be great to add:

dynamo version number
dynamoRevit version number
packages
dyn/dyf dependencies if the user wants them to be uploaded...

just things we have discussed, not that all that needs to go in this PR.

🎉

radumg · 2018-10-05T21:43:40Z

Thanks @mjkkirschner !

Dynamo version number is actually included in the issue title.

The rest could also be added to body of issue, my only concern is how feasible it is to use the simple url query params mechanism i'm using here to add that much info - haven't stress-tested it yet 💪

andydandy74 · 2018-10-05T21:46:21Z

Looks like 2kB of data is safe:
https://stackoverflow.com/questions/2659952/maximum-length-of-http-get-request

alfarok · 2018-10-26T16:30:30Z

Hey @radumg I just made sure this is in the backlog to get reviewed next sprint. It would be a nice improvement!

alfarok · 2018-10-26T16:31:41Z

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

 // by using the '*' as shown below:
 // [assembly: AssemblyVersion("1.0.*")]
-[assembly: AssemblyFileVersion("2.0.2.6067")]
+[assembly: AssemblyFileVersion("2.0.2.6426")]


These changes don't need to be committed and can be excluded

i'm not doing anything on purpose here (include or exclude) - do you mean I should manually exclude (not commit) them ? Feels like there should be something in .gitignore for it or an action step on build ?

alfarok · 2018-10-26T16:33:01Z

src/DynamoCoreWpf/Properties/Resources.Designer.cs

    // To add or remove a member, edit your .ResX file then rerun ResGen
    // with the /str option, or rebuild your VS project.
-    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "4.0.0.0")]
+    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "15.0.0.0")]


Is this update required? Are there any other repercussions to updating?

This is due to my VisualStudio 2017, i'll revert to be on the safe side.

alfarok · 2018-10-26T16:36:03Z

src/DynamoCoreWpf/Properties/Resources.resx

  </data>
  <data name="PackagePathAutoAddNotificationShortDescription" xml:space="preserve">
-    <value>A library (*.dll, *.ds) was recently imported into Dynamo. Its path was automatically added to "Settings > Manage Node and Package Paths..."</value>
+    <value>A library (*.dll, *.ds) was recently imported into Dynamo. Its path was automatically added to "Settings &gt; Manage Node and Package Paths..."</value>


Was this change required?

Visual Studio 2017 did this one, i can revert it.

alfarok · 2018-10-26T16:38:08Z

Took a quick peak, I just added a couple comments/questions but looks good. We will get it pulled next sprint for further testing.

radumg · 2018-10-30T12:48:41Z

Thanks @alfarok for the review, i'll fix up those issues now 👍

This partially reverts commits b6ca5d7. 64c75aa

since we make a new Uri, it would never have any existing query parameters

radumg · 2018-10-30T13:43:16Z

should be ready for review again @alfarok !

alfarok · 2018-10-30T14:12:53Z

Thanks for the updates @radumg, looks good. We will run the changes on the self-serve CI and get some manual testing in the upcoming sprint. It may also be worth adding a unit test calling ReportABug (without actually forcing a crash) just to verify everything is operating as expected.

radumg · 2018-10-30T14:44:36Z

Might get tricky/messy to detect a browser window opened correctly, but i could decompose the window launching & figuring out what to launch part, so we can unit test.

Or i could just check the return code (int) of Process.Start - which would be better suited you think ?

alfarok · 2018-11-05T15:18:52Z

Hey @radumg, I don't think it will be super easy test the issue text body but what about verifying the window is open and has the correct window title. I would try to call ReportABug(), verify the process is running Process.GetProcesses(), and use Process.MainWindowTitle to verify the expected window exists in the running processes. Let me know if that makes sense or if you run into any issues.

radumg · 2018-11-06T14:56:41Z

Yep, makes sense and i was thinking similar, so good to know that's all we'd need to check 👍
Now, because this launches user default browser, we have no way of knowing if it'll launch Chrome, IE, Firefox or Edge (or Opera for hipster users 😛). I'll look into it, though.

alfarok · 2018-11-06T20:08:11Z

I think no matter which browser gets launched the main window title should be consistent, such as New Issue - DynamoDS/Dynamo but I am not positive on that with tabs etc. Thanks for looking into this. It would be really great if we could make the self-serve CI publicly accessible in the future to make testing easier for everyone. Maybe Microsoft will give us some additional new options!

alfarok · 2018-11-08T18:19:50Z

Hey @radumg I just realized this PR is to RC2.0.2_master but it should be to master (2.1.0), no new features are currently going into 2.0.2. I just kicked off your branch on the self-serve CI to make sure all the existing tests are passing. I will also pull it down locally over the next couple days to do a little manual testing.

radumg · 2018-11-08T19:35:28Z

thanks @alfarok , will rebase on master then.

radumg · 2018-11-08T19:38:11Z

hmmm, changing base to master does not look quite ok, not sure which branch this should be based on then ?

I just picked the highest 2.0+ branch i found at the time 😁

alfarok · 2018-11-08T19:59:35Z

@radumg That's because you are diffing your fork of RC2.0.2_master against Master. These 2 branches have quite a few difference so you are seeing all of them when comparing. There are a couple options but the easiest is probably to cherry-pick your commits to master on your fork and open a new PR. (Let me know if you have any questions on this)

I also have some good news and bad news. The good news is all the tests are passing with your changes. The bad news is that pressing the Submit to Github button seems to crash the crash? After clicking, everything closes including Dynamo with no further activity. Looks like an exception is being thrown:

Previously the Dynamo and crash windows would remain open while the browser was opened allowing the user to save upon pressing Continue. This makes that test even more important because we should be getting a failure if the behavior has regressed. Since the self-serve is passing it means we have no code coverage on this functionality ☹.

The crash I was using to test was reported by @andydandy74 recently. Load the Core_Math sample, select all the nodes, use node to code. I also tested on master and the previous implementation launches the browser as expected and Dynamo remains open.

alfarok · 2018-11-08T20:14:53Z

It seems the issue is that Resources.CrashPromptGithubNewIssueTitle is returning null

radumg · 2018-11-08T21:19:08Z

Great, thanks so much @alfarok ! I can test this now knowing a reproducible crash.
I'll cherry-pick to new PR if I can't rebase more cleanly.

alfarok · 2018-11-26T19:26:14Z

Hey @radumg, just checking in to see if there are any updates here? The testing task in our last sprint closed so just let me know if this needs another look and I will make sure it gets into the following sprint.

radumg · 2018-11-27T08:04:41Z

Hey Keith @alfarok , haven’t managed to look at it post-AU (backlog..😖) but will set some time aside on Friday and let you know how I get on 🙂

radumg · 2019-01-09T12:49:17Z

@alfarok , made a new PR

based on latest master branch
adds unit test
fixes null check

radumg added 3 commits October 5, 2018 20:01

add /new to GitHubBugReportingLink

b6ca5d7

you want to make it easy for users to report the issue, not take them to a page where they stare at a wall of 600 unsolved issues and having to figure out HOW to open/submit a new one

add bug report title to .resx

64c75aa

add support for pre-filled new Github issue text when reporting

2caa718

radumg mentioned this pull request Oct 5, 2018

Bug-reporting UX #5909

Closed

alfarok reviewed Oct 26, 2018

View reviewed changes

radumg added 3 commits October 30, 2018 13:16

Revert assembly & .resx un-necessary changes

a1a96fc

This partially reverts commits b6ca5d7. 64c75aa

simplify query construction

bf955c0

since we make a new Uri, it would never have any existing query parameters

rename variables for legibility

e279d5f

radumg changed the base branch from RC2.0.2_master to master November 8, 2018 19:36

radumg changed the base branch from master to RC2.0.2_master November 8, 2018 19:36

radumg mentioned this pull request Jan 9, 2019

5909 report bug ux tested #9393

Merged

7 tasks

radumg closed this Jan 9, 2019

radumg deleted the 5909-Bug-reporting-UX branch January 9, 2019 12:57

horatiubota added the error/warning/crash Issues mentioning a Dynamo error, warning or crash message label Nov 11, 2019

5909 pre-fill Github issue reports with crash details #9150

5909 pre-fill Github issue reports with crash details #9150

Uh oh!

Conversation

radumg commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Other notes

Further improvements

Declarations

Reviewers

FYIs

Uh oh!

mjkkirschner commented Oct 5, 2018

Uh oh!

radumg commented Oct 5, 2018

Uh oh!

andydandy74 commented Oct 5, 2018

Uh oh!

alfarok commented Oct 26, 2018

Uh oh!

alfarok Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

radumg Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfarok Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

radumg Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

alfarok Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

radumg Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

alfarok commented Oct 26, 2018

Uh oh!

radumg commented Oct 30, 2018

Uh oh!

radumg commented Oct 30, 2018

Uh oh!

alfarok commented Oct 30, 2018

Uh oh!

radumg commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alfarok commented Nov 5, 2018

Uh oh!

radumg commented Nov 6, 2018

Uh oh!

alfarok commented Nov 6, 2018

Uh oh!

alfarok commented Nov 8, 2018

Uh oh!

radumg commented Nov 8, 2018

Uh oh!

radumg commented Nov 8, 2018

Uh oh!

alfarok commented Nov 8, 2018

Uh oh!

alfarok commented Nov 8, 2018

Uh oh!

radumg commented Nov 8, 2018

Uh oh!

alfarok commented Nov 26, 2018

Uh oh!

radumg commented Nov 27, 2018

Uh oh!

radumg commented Jan 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

radumg commented Oct 5, 2018 •

edited

Loading

radumg Oct 30, 2018 •

edited

Loading

radumg commented Oct 30, 2018 •

edited

Loading