Proposed type of test: chrome Proposed location of test: toolkit/mozapps/downloads/tests/chrome

Comment on attachment 328129 [details] [diff] [review] v1.0 general nit: s/var/let/ please general nit: this file has tabs in it. Please remove them. general nit: you have lots of trailing spaces in this file. Please remove them >+/** This is an automated version of the litmus test, written for bug 443585. >+ * Basically this test downloads 4 files named - "one", "two", "three" and >+ * "one two three". This test checks that while we enter the * string "one >+ * two three" into the search box, we get to see only the download "one two >+ * three" amongst the results >+ */ nit: formatting should be like this: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_basic_functionality.xul#40 >+<window title="Download Manager UI Test Code" >+ onload="setTimeout(runTest, 0);" >+ xmlns:html="http://www.w3.org/1999/xhtml" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> why the timeout? nit: line up attributes like so: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_basic_functionality.xul#47 >+ <title>Download Manager UI Test</title> Could you give a better title perhaps? >+function populateDM() >+{ >+ let db = dm.DBConnection; >+ >+ // First, we populate the database with some fake data >+ db.executeSimpleSQL("DELETE FROM moz_downloads"); >+ let rawStmt = db.createStatement( >+ "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " + >+ "state, currBytes, maxBytes, preferredAction, autoResume) " + >+ "VALUES (:name, :source, :target, :startTime, :endTime, :state, " + >+ ":currBytes, :maxBytes, :preferredAction, :autoResume)"); nit: really weird spacing. Try like this when you address next comment: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_basic_functionality.xul#191 >+ let stmt = Cc["@mozilla.org/storage/statement-wrapper;1"]. >+ createInstance(Ci.mozIStorageStatementWrapper) >+ stmt.initialize(rawStmt); >+ for each (let dl in DownloadData) { >+ for (let prop in dl) >+ stmt.params[prop] = dl[prop]; >+ >+ stmt.execute(); >+ } >+ stmt.statement.finalize(); >+ >+ return; >+} This should be pulled out and added to http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/utils.js. You'll have to include this file too (some other test files do - you can get the path from there). >+ >+ >+function testSearchResults() >+{ >+ let dm_win = wm.getMostRecentWindow("Download:Manager"); >+ let richlistbox = dm_win.document.getElementById("downloadView"); >+ let searchbox = dm_win.document.getElementById("searchbox"); >+ >+ searchbox.value = "one two three"; >+ >+ ok(richlistbox.itemCount == 1, "The search box shows other downloads(which are named " + >+ + "as 'one', 'two', 'three'), apart from the download called" + >+ + " 'one two three' which has been entered as the search string"); >+ >+ cleanup(); >+ >+ return; >+} This won't actually work. Please see http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_multiword_search.xul (although, why aren't you just adding to this test in the first place?) >+cleanUp() >+{ >+ let dm_win = wm.getMostRecentWindow("Download:Manager"); >+ if (dm_win) dm_win.close(); >+ >+ dmFile.remove(false); >+ dm.DBConnection.executeSimpleSQL("DELETE FROM moz_downloads"); >+ >+ SimpleTest.finish(); >+ >+ return; >+} This should be in utils.js too. Probably called "setCleanState". Ditch the finish call, and there is no need for a return statement. >+function runTest() >+{ >+ const DLMGR_UI_DONE = "download-mangager-ui-done"; >+ >+ let dm_win = wm.getMostRecentWindow("Download:Manager"); We can ditch the window mediator all together and add a function in utils.js for this too. It's a common operation that you'll be doing a lot in your other tests too.

Additionally, once you pull a lot of the common boilerplate code out into utils.js, I'd like to see just a runTest method that calls the right stuff. There's really no good reason to change the common test style in that directory (other than making it harder for new people to look at existing tests to figure out how to write a new one)

Actually I can add this to test_multiword_search.xul. I want to discuss this with you once you are online. If it is resolved that it should enter test_multiword_search.xul I will do it

Comment on attachment 329435 [details] [diff] [review] v2.0 (In reply to comment #4) > Actually I can add this to test_multiword_search.xul. I want to discuss this > with you once you are online. If it is resolved that it should enter > test_multiword_search.xul I will do it It should be a part of it - I thought we had discussed this over im.

This test won't work. I think I need to add a listener to the richlistbox and wait for it to remove its items and then carry out a check. Please keep this review in pending state.

This test won't work. I think I need to add a listener to the richlistbox and wait for it to remove its items and then carry out a check. Please keep this review in pending state.

Comment on attachment 329619 [details] [diff] [review] v5.0 See comment 5. Please make this a part of http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_multiword_search.xul (or explain why it cannot be)

It can be. I have updated test_multiword_search.xul with one more litmus test. This can be added :)