5909 report bug ux tested #9393

radumg · 2019-01-09T12:47:28Z

Purpose

NOTE : replaces PR #9150 which was based on wrong branch and adds unit tests.

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

@alfarok
@Racel

FYIs

@andydandy74 , @johnpierson

Update from DynamoDS repo

update from DynamoDS

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

radumg · 2019-01-09T12:52:40Z

Started on the unit tests @alfarok , will finish them later today, they test :

new browser tab is opened when a bug with no content is reported
new browser tab is opened when a bug with content is reported

For now, confirmed (in a separate experiment) the methodology works for all major browser (on Windows at least) and correctly catches open tabs with right name on : Edge, Chrome, Firefox & even Internet Explorer.

One potential hiccup is that if you're not logged in, you'd be taken to sign-in page, so i'll test further for that and adjust.

radumg · 2019-01-09T12:53:42Z

let me know in the meantime if it makes sense where the unit test is placed.

test/DynamoCoreWpfTests/CrashReportingTests.cs

mjkkirschner · 2019-01-09T19:24:34Z

@radumg sorry, I should have been more clear. Please keep the test class and tests public (I think they need to be public to be found by nunit on our runner) - I only meant to make the TargetWords list private.

😄

radumg · 2019-01-10T08:42:55Z

@mjkkirschner hah, thought there was something funny there, but looked at other classes and they lacked an access modifier, turns out only the methods need it. Didn't have the NUnit 2 adapter anyway, so couldn't test. 😁

update : confirm it picks up Dynamo version in issue title

radumg · 2019-01-10T11:52:57Z

Ok, should be ready for review now @mjkkirschner @alfarok

Tests passing

Works both logged in and logged out of Github

Now adds long stack trace & further metadata details (OS, CLR & Dynamo versions) to issue body

When submitted, it looks like this

Let me know what you think & if you have any feedback/ideas, etc 😄

radumg · 2019-01-10T11:55:47Z

FYI, introduced a new CrashUtilities.cs in Wpf.Utilities, it's set to internal to not impact public API, hope that's ok - if not, open to suggestions. Didn't want to polute the ViewModel with crash formatting implementation details.

alfarok · 2019-01-10T15:57:45Z

Hey @radumg, thanks for continuing this work! I'll pull your changes today and review

src/DynamoCoreWpf/Properties/Resources.Designer.cs

src/DynamoCoreWpf/Utilities/CrashUtilities.cs

radumg · 2019-01-10T21:28:07Z

Looking great 👏 , thanks @alfarok !

alfarok · 2019-01-11T16:04:00Z

@radumg quick update, the tests are failing on the test machine because no user is logged in so it brings you to the login page as you outlined. We can probably just test the body of the request instead and not open the browser at all. I am looking into a solution now

radumg · 2019-01-11T16:44:17Z

hm, interesting @alfarok ! There's a fallback so the unit test should detect it's on the sign-in page and pass.

In CrashReportingTests.cs :

private bool ContainsAllTargetWords(string source)
        {
            return TargetWords.TrueForAll(x => source.ToUpper().Contains(x)) ||
                   TargetWordsLoggedOutFallback.TrueForAll(x => source.ToUpper().Contains(x));
        }

where the words are :

List<string> TargetWordsLoggedOutFallback = new List<string> { "SIGN IN", "GITHUB" };

Running it here works logged out too.

radumg · 2019-01-11T16:45:07Z

@radumg quick update, the tests are failing on the test machine

What OS is the test machine ? Linux or Windows ? I'm wondering if grabbing the Process.MainWindowTitle is inconsistent between them

alfarok · 2019-01-11T18:31:03Z

@radumg I think it is because the test machine opens an additional window about setting a default browser (or something similar) before continuing. Unfortunately I don't think our testing machines will be super reliable with browser interactions, sorry for any additional work. I am making a PR to your branch now that simplifies the tests a bit but covers similar functionality. If we get everything resolved by today we should be able to get this into 2.1

alfarok · 2019-01-11T18:58:53Z

radumg#7

Address browser testing issues on test machines

radumg · 2019-01-11T20:37:01Z

All merged in 👍

radumg · 2019-01-11T20:47:02Z

Not sure what’s going with Github, my comments keep disappearing, so just in case : @alfarok your PR should be merged now, if not please feel free to proceed with it, all looks 👏

alfarok · 2019-01-11T21:21:03Z

@radumg once the self-serve completes I think this is ready to merge if there are no objections from @mjkkirschner. Thanks for the quick updates the last couple of days, this will be really nice to have in 2.1 👍

src/DynamoCoreWpf/Utilities/CrashUtilities.cs

mjkkirschner · 2019-01-11T21:36:32Z

this looks good to me.

alfarok · 2019-01-11T22:47:30Z

@radumg all unit tests passing, merged. @QilongTang can I cherry-pick to 2.1?

* add /new to GitHubBugReportingLink url * add bug report title to .resx * add support for pre-filled new Github issue text when reporting * Revert resx un-necessary changes * simplify query construction since we make a new Uri, it would never have any existing query parameters * rename variables for legibility * add fallback values for text to prevent null exceptions * invert IF logic to bail early * add XML comment for public method * get rid of conditional logic ✌️ * scaffold unit tests for CrashReporting * add CrashReportingTests.cs to DynamoCoreWpfTests proj * remove public access modifiers * add logged out fallback and make tab detection case-insensitive * move issue text & url formatting to new CrashUtilities.cs file * add test for long stack trace * Revert "Revert resx un-necessary changes" This reverts commit 01fc93c. * add description to issue crash report * fix XML comments * further formatting to crash report * Include issue details from old template * additional new line required * remove browser interaction from unit tests * decrement Dynamo version to make release * comment spelling * spelling - GithhubCrashReportBody becomes GitHubCrashReportBody

radumg · 2019-01-11T23:29:08Z

Thanks for the quick updates the last couple of days, this will be really nice to have in 2.1 👍

My pleasure, many thanks @alfarok and @mjkkirschner for the help 🍻

* add /new to GitHubBugReportingLink url * add bug report title to .resx * add support for pre-filled new Github issue text when reporting * Revert resx un-necessary changes * simplify query construction since we make a new Uri, it would never have any existing query parameters * rename variables for legibility * add fallback values for text to prevent null exceptions * invert IF logic to bail early * add XML comment for public method * get rid of conditional logic ✌️ * scaffold unit tests for CrashReporting * add CrashReportingTests.cs to DynamoCoreWpfTests proj * remove public access modifiers * add logged out fallback and make tab detection case-insensitive * move issue text & url formatting to new CrashUtilities.cs file * add test for long stack trace * Revert "Revert resx un-necessary changes" This reverts commit 01fc93c. * add description to issue crash report * fix XML comments * further formatting to crash report * Include issue details from old template * additional new line required * remove browser interaction from unit tests * decrement Dynamo version to make release * comment spelling * spelling - GithhubCrashReportBody becomes GitHubCrashReportBody

This reverts commit dfb7ddf.

This reverts commit 8ecd628.

* 5909 report bug ux tested (#9393) * add /new to GitHubBugReportingLink url * add bug report title to .resx * add support for pre-filled new Github issue text when reporting * Revert resx un-necessary changes * simplify query construction since we make a new Uri, it would never have any existing query parameters * rename variables for legibility * add fallback values for text to prevent null exceptions * invert IF logic to bail early * add XML comment for public method * get rid of conditional logic ✌️ * scaffold unit tests for CrashReporting * add CrashReportingTests.cs to DynamoCoreWpfTests proj * remove public access modifiers * add logged out fallback and make tab detection case-insensitive * move issue text & url formatting to new CrashUtilities.cs file * add test for long stack trace * Revert "Revert resx un-necessary changes" This reverts commit 01fc93c. * add description to issue crash report * fix XML comments * further formatting to crash report * Include issue details from old template * additional new line required * remove browser interaction from unit tests * decrement Dynamo version to make release * comment spelling * spelling - GithhubCrashReportBody becomes GitHubCrashReportBody * Revert "5909 report bug ux tested (#9393)" This reverts commit dfb7ddf.

radumg and others added 14 commits January 3, 2018 00:02

Merge pull request #3 from DynamoDS/master

09556d2

Update from DynamoDS repo

Merge pull request #4 from DynamoDS/master

1ecb8c7

update from DynamoDS

add /new to GitHubBugReportingLink url

a14e129

add bug report title to .resx

a0fe24e

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

b9f5203

Revert resx un-necessary changes

01fc93c

simplify query construction

16a440f

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

rename variables for legibility

498bb71

add fallback values for text to prevent null exceptions

a7802a4

invert IF logic to bail early

008dce6

add XML comment for public method

8749989

get rid of conditional logic ✌️

8baf08e

scaffold unit tests for CrashReporting

037c2ae

add CrashReportingTests.cs to DynamoCoreWpfTests proj

83b5c9a

mjkkirschner reviewed Jan 9, 2019

View reviewed changes

test/DynamoCoreWpfTests/CrashReportingTests.cs Outdated Show resolved Hide resolved

remove public access modifiers

ad685cb

add logged out fallback and make tab detection case-insensitive

45ed299

radumg added 2 commits January 10, 2019 11:44

move issue text & url formatting to new CrashUtilities.cs file

a2f72fd

add test for long stack trace

b7bb3f0

alfarok reviewed Jan 10, 2019

View reviewed changes

src/DynamoCoreWpf/Properties/Resources.Designer.cs Outdated Show resolved Hide resolved

alfarok reviewed Jan 10, 2019

View reviewed changes

src/DynamoCoreWpf/Properties/Resources.Designer.cs Show resolved Hide resolved

alfarok reviewed Jan 10, 2019

View reviewed changes

src/DynamoCoreWpf/Utilities/CrashUtilities.cs Outdated Show resolved Hide resolved

alfarok reviewed Jan 10, 2019

View reviewed changes

src/DynamoCoreWpf/Utilities/CrashUtilities.cs Outdated Show resolved Hide resolved

alfarok added 2 commits January 11, 2019 13:27

remove browser interaction from unit tests

af46c9b

decrement Dynamo version to make release

fb9d0ed

Merge pull request #7 from alfarok/5909-report-bug-ux-tested

ca75045

Address browser testing issues on test machines

comment spelling

1d390c8

mjkkirschner reviewed Jan 11, 2019

View reviewed changes

src/DynamoCoreWpf/Utilities/CrashUtilities.cs Outdated Show resolved Hide resolved

spelling - GithhubCrashReportBody becomes GitHubCrashReportBody

626acd0

alfarok added the LGTM Looks good to me label Jan 11, 2019

alfarok merged commit fc7d5a3 into DynamoDS:master Jan 11, 2019

alfarok mentioned this pull request Jan 11, 2019

[Cherry-picked] 5909 report bug ux tested (#9393) #9414

Merged

alfarok added a commit to alfarok/Dynamo that referenced this pull request Jan 14, 2019

Revert "5909 report bug ux tested (DynamoDS#9393)"

93ead57

This reverts commit dfb7ddf.

alfarok mentioned this pull request Jan 14, 2019

Revert #9393 on RC2.1.0 branch #9420

Merged

7 tasks

reddyashish added a commit that referenced this pull request Jan 14, 2019

Revert "5909 report bug ux tested (#9393) (#9414)"

a9dc83f

This reverts commit 8ecd628.

This was referenced Feb 25, 2019

Warn users submitting crash reports that they must be signed into GitHub #9519

Closed

Exclude stack trace from being included in the new GitHub issues request #9523

Merged

QilongTang mentioned this pull request Jan 28, 2020

Add missing resource string #10329

Merged

7 tasks

5909 report bug ux tested #9393

5909 report bug ux tested #9393

Uh oh!

Conversation

radumg commented Jan 9, 2019 • 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!

radumg commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radumg commented Jan 9, 2019

Uh oh!

Uh oh!

mjkkirschner commented Jan 9, 2019

Uh oh!

radumg commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radumg commented Jan 10, 2019

Uh oh!

radumg commented Jan 10, 2019

Uh oh!

alfarok commented Jan 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

radumg commented Jan 10, 2019

Uh oh!

alfarok commented Jan 11, 2019

Uh oh!

radumg commented Jan 11, 2019

Uh oh!

radumg commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alfarok commented Jan 11, 2019

Uh oh!

alfarok commented Jan 11, 2019

Uh oh!

radumg commented Jan 11, 2019

Uh oh!

radumg commented Jan 11, 2019

Uh oh!

alfarok commented Jan 11, 2019

Uh oh!

Uh oh!

mjkkirschner commented Jan 11, 2019

Uh oh!

alfarok commented Jan 11, 2019

Uh oh!

radumg commented Jan 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

radumg commented Jan 9, 2019 •

edited

Loading

radumg commented Jan 9, 2019 •

edited

Loading

radumg commented Jan 10, 2019 •

edited

Loading

radumg commented Jan 11, 2019 •

edited

Loading