Closed Bug 901126 Opened 8 years ago Closed 8 years ago

Split browser_newtab_drag_drop.js into two tests

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: ttaubert, Assigned: iforgot120)

References

Details

(Whiteboard: [good first bug][mentor=ttaubert][lang=js])

Attachments

(1 file, 5 obsolete files)

We should split browser/base/content/test/newtab/browser_newtab_drag_drop.js into two tests because there are a couple of reports that this is running longer than allowed.

The easiest way to do that would be to create a second test named browser_newtab_drag_drop_ext.js (for example) and move the second half that uses simulateExternalDrop() to it.
Component: General → Tabbed Browser
Whiteboard: [good first bug][mentor=ttaubert][lang=js]
I can work on this. What's the ideal/preferred max running time for a test?
The default timeout in our test suite is 30s but tests usually run a lot slower on our test machines. I think splitting this test in two as a first step is a good thing to do. We can always split it more shouldn't that be sufficient.
Sorry for the delay! I'll start working on this, then. If the tests still take longer than 30s, should I just go ahead and split them further?
Blocks: 904948
Don't worry about that for now. Splitting the test in two should be good enough for a first start!
I split up the .js test file into two parts. How should I test them out? Is there a test server?
Great! You can run all the tests locally by doing the following:

./mach mochitest-browser browser/base/content/test/newtab/

You should execute this in a console, in your checkout directory of the source tree. If any of the tests fail you should see "TEST-UNEXPECTED-FAIL" error messages in red.
It should run every test in the /newtab folder, right? I don't have to add the new test file to any list somewhere?

What's the easiest way to share the test log? It says that 108 tests were passed, but one failed, and not the one in this bug:


0:50.67 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_focus.js | Validate focus count in the new tab page. - Got 29, expected 28
Attached patch bug901126.patch (obsolete) — Splinter Review
Still new to making patches - hope this one works okay.
Attached patch more clear on the edits (obsolete) — Splinter Review
Sorry for the patch spam.
(In reply to velocirabbit from comment #7)
> It should run every test in the /newtab folder, right? I don't have to add
> the new test file to any list somewhere?

You don't need to run all the tests, you can also just run the tests you added. It will unfortunately not run all the tests in the folder, you will need to add browser_newtab_drag_drop_ext.js to the list in browser/base/content/test/newtab/Makefile.in.

> 0:50.67 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/newtab/
> browser_newtab_focus.js | Validate focus count in the new tab page. - Got
> 29, expected 28

Hmm that failure is odd but I wouldn't care too much about it. You can just run each of your tests by its own.
Attachment #791134 - Attachment is obsolete: true
Comment on attachment 791135 [details] [diff] [review]
more clear on the edits

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

That approach looks great and is exactly what I had in mind!

Unfortunately the patch format doesn't look quite right. What did you use to create the patch/diff? Here's a little help on how to generate patches https://developer.mozilla.org/en-US/docs/Mercurial_FAQ

Thanks!

::: browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js
@@ +11,5 @@
> + */
> +function runTest() {
> +	// drag a new site onto the very first cell
> +	yield setLinks("0,1,2,3,4,5,6,7,8");
> +	setPinnedLinks(",,,,,,,7,8");

Please use two spaces for indentation, we never use tabs to do that. You might want to check your editor settings to map your <TAB> key to insert spaces automatically when indenting.
Attachment #791135 - Flags: feedback+
Assignee: nobody → iforgot120
Status: NEW → ASSIGNED
Attachment #791135 - Attachment is obsolete: true
Comment on attachment 791539 [details] [diff] [review]
Okay! This should work better. Let me know if I need to change it again.

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

This looks great, thank you! Let me push this to try to check that it works as expected.
Attachment #791539 - Flags: review+
Comment on attachment 791539 [details] [diff] [review]
Okay! This should work better. Let me know if I need to change it again.

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

::: browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js
@@ +8,5 @@
> + * a new site into it another one gets pushed out.
> + * This is a continuation of browser_newtab_drag_drop.js
> + * to decrease test run time, focusing on external sites.
> + */
> +function runTest() {

That try run didn't go well, unfortunately. The function needs to be named runTests(). Sorry, I didn't notice that when reviewing. Andrew, can you please update your patch? I'll push it to try again then.
Attachment #791539 - Flags: review+ → review-
Attachment #791539 - Attachment is obsolete: true
Comment on attachment 792576 [details] [diff] [review]
Whoops! Don't know how I missed that.

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

Thanks Andrew, this looks great! Can you please prepare the patch for checkin-needed? That means giving it a description like "Bug 12345 - Test bug; r=reviewer", uploading it again and adding the 'checkin-needed' keyword to the bug.
Attachment #792576 - Flags: review+
checkin-needed
Attachment #792576 - Attachment is obsolete: true
Like that? There wasn't a field specifically for keywords.
Sorry, I meant the patch itself should have a new description. It should of course not be "Test bug", and "reviewer". You should describe what the patch does and add r=ttaubert, sorry I should have been a little clearer about that.

Here is a good writeup on how to do that:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Also, checkin-needed is a flag that you can set in the "Keywords" field at the top of this bug when the patch is ready.
Attached patch bug901126.patchSplinter Review
Should be all updated now!
Attachment #792769 - Attachment is obsolete: true
Keywords: checkin-needed
That looks great, thank you! One of our sheriffs will pick this up soon and land it.
https://hg.mozilla.org/integration/fx-team/rev/4eb0c1631e83
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=ttaubert][lang=js] → [good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team]
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/4eb0c1631e83
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team] → [good first bug][mentor=ttaubert][lang=js]
Target Milestone: --- → Firefox 26
Depends on: 911107
You need to log in before you can comment on or make changes to this bug.