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.

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?

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

Still new to making patches - hope this one works okay.

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.

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.

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.

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.

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.

checkin-needed

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.

Should be all updated now!

That looks great, thank you! One of our sheriffs will pick this up soon and land it.

https://hg.mozilla.org/releases/mozilla-aurora/rev/3987a287ccc8 https://hg.mozilla.org/releases/mozilla-beta/rev/ab944ea10a88