Closed Bug 934207 Opened 6 years ago Closed 6 years ago

Add browser_thumbnails_background_crash.js to browser.ini

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Looks like the test is run when running all of the thumbnail tests but it's impossible to run it in single mode.
Attachment #826400 - Flags: review?(mhammond)
Attachment #826400 - Flags: review?(adw)
Comment on attachment 826400 [details] [diff] [review]
0005-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

Review of attachment 826400 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/test/browser.ini
@@ +11,5 @@
>  
>  [browser_thumbnails_background.js]
>  # Too many intermittent failures (bug 931889)
>  skip-if = os == "linux"
> +[browser_thumbnails_background_crash.js]

This will need |run-if = crashreporter| and Makefile.in should be removed (as these are the last 2 there) - see bug 932147 for where I'm doing the same in a different dir.  r=me for that, but I think it's probably best to upload a new patch with that, carry my r+ forward and request review from jhammel for the removal of the Makefile (r+ is pending from him in bug 932147, otherwise I'd just say "go for it" :)
Attachment #826400 - Flags: review?(mhammond)
Attachment #826400 - Flags: review?(adw)
Attachment #826400 - Flags: review+
(In reply to Mark Hammond [:markh] from comment #2)
> This will need |run-if = crashreporter|

Does it? I build without the crash reporter locally and it runs just fine. The test checks whether the crash reporter is available.

> and Makefile.in should be removed

Ah, thanks. That's where the test was. Removed Makefile.in and asking jhammel for review as well.
Attachment #826400 - Attachment is obsolete: true
Attachment #826681 - Flags: review?(mhammond)
Attachment #826681 - Flags: review?(jhammel)
Comment on attachment 826681 [details] [diff] [review]
0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

Review of attachment 826681 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm;
Attachment #826681 - Flags: review?(jhammel) → review+
(In reply to Tim Taubert [:ttaubert] from comment #3)
> Created attachment 826681 [details] [diff] [review]
> 0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch
> 
> (In reply to Mark Hammond [:markh] from comment #2)
> > This will need |run-if = crashreporter|
> 
> Does it? I build without the crash reporter locally and it runs just fine.
> The test checks whether the crash reporter is available.

We explicitly added that condition to the Makefile in bug 919438 - I really can't recall much history (ie, why it was a problem on ASAN) other than it failed - Drew, can you remember?
Flags: needinfo?(adw)
Christian mentioned it in bug 915384 comment 77.  Christian, do you know whether that's still accurate, given the conversation in this bug (see comment 3, comment 5)?
Flags: needinfo?(adw) → needinfo?(choller)
FTR, /browser/base/content/test/social/browser_social_workercrash.js does the same |if ('nsICrashReporter' in Ci)| check and it's not using |run-if = crashreporter|.
If you modified this test since bug 919438 to properly check for the crash reporter on its own, then it might not be required anymore to check it in browser.ini. If you want to make sure, just push it to try with linux-asan and you'll see if it fails the ASan builds. If that is green, then you can just go ahead and omit the run-if = crashreporter stuff.
Flags: needinfo?(choller)
Comment on attachment 826681 [details] [diff] [review]
0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

https://hg.mozilla.org/integration/fx-team/rev/c80b984d7c5c
Attachment #826681 - Flags: review?(mhammond)
https://hg.mozilla.org/mozilla-central/rev/c80b984d7c5c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Is there any help needed for manual testing?
Flags: needinfo?(ttaubert)
No, seems to be running fine on our test infrastructure. Thanks!
Flags: needinfo?(ttaubert)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.