1422697 - Fix detection of CrashReporter for test_crash.py

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Bug 1402519 made the crash reporter interface available all the time. As such our check here doesn't work anymore:

https://dxr.mozilla.org/mozilla-central/rev/574f4f58fe09dd590ea892406e237318c31705b4/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py#28-36

Instead we have to import `AppConstants.jsm`, and check for the crash reporter enabled state via:

> Cu.import("resource://gre/modules/AppConstants.jsm");
> if (AppConstants.MOZ_CRASHREPORTER) {
>  // Crash reporter is enabled
> }

Comment 1

[Mohit Agrawal (:mAgr)]

HI!
Can I start to work on this?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mohit, sorry, but Vedant is already working on this particular bug. He only missed to comment.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mohit, but there is bug 1423103, which will be JS, but maybe you have interest.

Comment 4

[Vedant Chakravadhanula(:vedantc98)]

Sorry I didn't put out a comment on this.
I'm currently working on this bug.

Comment 5

[Vedant Chakravadhanula(:vedantc98)]

Attachment: Bug 1422697 - Changed crash reporter detection in unit test

Since the crash reporter interface was made available all the time, the check for the crash reporter had to be made via AppConstants.jsm

Review commit: https://reviewboard.mozilla.org/r/205444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205444/

Comment 6

[Vedant Chakravadhanula(:vedantc98)]

Hey, I've pushed a patch for review, but there does seem to be an issue.
I changed the application.ini file to remove the [Crash Reporter] portion, but for some reason the variable self.crash_reporter_enabled still returns true inspite of disabling(apparently) the crash reporter.

Could you help me out a little with this? Am I not disabling the reporter correctly?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8934536 [details]
Bug 1422697 - Changed crash reporter detection in unit test

https://reviewboard.mozilla.org/r/205444/#review211182

This looks basically fine. Lets fix the commit message and we should be fine. I will trigger a try build in the meantime. Thanks!

::: commit-message-b4cef:3
(Diff revision 1)
> +Bug 1422697 - Changed the detection of crash reporter for crash_test.py. r=whimboo
> +
> +Since the crash reporter interface was made available all the time, the check for the crash reporter had to be made via AppConstants.jsm

Please trim the line length of the detailed description to around 72 characters.

Also name it that way which sounds better: `the check for the enabled state of the crash reporter has to be made via AppConstants.jsm`.

Comment 8

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8934536 [details]
Bug 1422697 - Changed crash reporter detection in unit test

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205444/diff/1-2/

Comment 9

[Vedant Chakravadhanula(:vedantc98)]

I've updated the commit message. Please let me know if anything else is required. Thanks!

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8934536 [details]
Bug 1422697 - Changed crash reporter detection in unit test

https://reviewboard.mozilla.org/r/205444/#review211182

> Please trim the line length of the detailed description to around 72 characters.
> 
> Also name it that way which sounds better: `the check for the enabled state of the crash reporter has to be made via AppConstants.jsm`.

As mentioned please keep a line length of about 72 characters. It still exceeds even 80 chars.

Comment 11

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8934536 [details]
Bug 1422697 - Changed crash reporter detection in unit test

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205444/diff/2-3/

Comment 12

[Vedant Chakravadhanula(:vedantc98)]

I'm sorry, I thought you meant the initial line of the commit message. I've changed the detailed description to be less than 72 characters, but it does sound a little abrupt.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8934536 [details]
Bug 1422697 - Changed crash reporter detection in unit test

https://reviewboard.mozilla.org/r/205444/#review211182

> As mentioned please keep a line length of about 72 characters. It still exceeds even 80 chars.

My request was actually not to change the content, but to limit the line length. So please revert the last change, and add line breaks manually.

Comment 14

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8934536 [details]
Bug 1422697 - Changed crash reporter detection in unit test

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205444/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/240069ba65d7
Changed crash reporter detection in unit test r=whimboo

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you Vedant for the patch. I just got it landed.

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/240069ba65d7