Closed Bug 931889 Opened 8 years ago Closed 8 years ago

Fix and re-enable browser_thumbnails_background.js

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: emorley, Assigned: adw)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

browser_thumbnails_background.js is really unreliable, particularly on Linux. See dependent bugs.

As such, it has been disabled on Linux for now.
Disabled on Linux by:
remote:   https://hg.mozilla.org/mozilla-central/rev/e7439c6c1e81
Whiteboard: [test disabled on linux] [leave open]
Attached patch split 'r up (obsolete) — Splinter Review
This splits up the test like we talked about.  It also:

* Modifies the bg tests to use helpers in head.js instead of their own where
  possible
* Adds helpers to head.js
* Modifies next() and TestRunner.next() in head.js to take arguments and pass
  them to the yielder via _iter.send()
* Drops the privateBrowsingActive and openPrivateWindowDuringCapture tests
  since I don't think they're useful anymore, but if you'd like, I can add
  them back
Attachment #8334373 - Flags: review?(mhammond)
iter.send(undefined) works on a newborn iterator, so this head.js change:

>       let value = aValue ? TestRunner._iter.send(aValue) :
>                   TestRunner._iter.next();

can be this instead, since TestRunner.run() calls this.next() with no arguments:

>       let value = TestRunner._iter.send(aValue);

I'll batch that change with any other comments you might have.
Comment on attachment 8334373 [details] [diff] [review]
split 'r up

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

looks good, sorry for the earlier confusion.

::: toolkit/components/thumbnails/test/browser_thumbnails_bg_queueing.js
@@ +9,5 @@
> +    bgTestPageURL({ wait: 2002 }),
> +    "http://www.example.com/2",
> +  ];
> +  urls.forEach(u => ok(!thumbnailExists(u), "Thumbnail should not exist yet"));
> +  urls.forEach(function (url) {

might as well use a fat-arrow callback here too

::: toolkit/components/thumbnails/test/head.js
@@ +54,2 @@
>      try {
> +      let value = aValue ? TestRunner._iter.send(aValue) :

yeah, the simpler version you mentioned would be better.
Attachment #8334373 - Flags: review?(mhammond) → review+
https://tbpl.mozilla.org/?tree=Try&rev=6d92aa1ec693
Assignee: nobody → adw
Status: NEW → ASSIGNED
Tryserver didn't show any oranges that weren't already there, so this is ready to land.  After that, I think this bug can be closed and new bugs filed against any new failing tests if necessary.
Attachment #8334373 - Attachment is obsolete: true
Er, I forgot about the many dependent bugs.  I think each can be rescoped to whatever test file they now happen to be failing in.
https://hg.mozilla.org/integration/fx-team/rev/c189beb8d5f5
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [test disabled on linux] [leave open] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c189beb8d5f5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Is there any help needed for manual testing?
Flags: needinfo?(adw)
No.
Flags: needinfo?(adw)
Depends on: 947809
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.