Closed Bug 1422697 Opened 2 years ago Closed 2 years ago

Fix detection of CrashReporter for test_crash.py

Categories

(Testing :: Marionette, enhancement)

59 Branch
enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: whimboo, Assigned: vedantc98, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js][lang=py])

User Story

Please check the following documentation in how to get started:

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

Attachments

(1 file)

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
> }
HI!
Can I start to work on this?
Mohit, sorry, but Vedant is already working on this particular bug. He only missed to comment.
Mohit, but there is bug 1423103, which will be JS, but maybe you have interest.
Sorry I didn't put out a comment on this.
I'm currently working on this bug.
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?
Flags: needinfo?(hskupin)
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`.
Attachment #8934536 - Flags: review?(hskupin) → review+
Flags: needinfo?(hskupin)
I've updated the commit message. Please let me know if anything else is required. Thanks!
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.
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 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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/240069ba65d7
Changed crash reporter detection in unit test r=whimboo
Thank you Vedant for the patch. I just got it landed.
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/240069ba65d7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.