Implement tests for the sqlite -> json download migration

Hi Paolo. Apologies for the delay, it took me a while to figure out all the details on how to make this work properly.. But I think it's close now. There's one more thing that I'm at a loss and can't figure out, so I'm posting here the patch to ask for your help on that. I left a comment in the patch, but briefly: after I import a download that will autoResume, I need to wait for it to finish, but I've been unable to figure out how to do so. I tried using the promiseDownloadStopped function but that it doesn't work (it always returns a rejected promise).. In the meantime the workaround I used to run the tests is that I added a `yield` in DownloadImport so it will wait by itself for the download to finish There are a couple of other simpler questions that I left a XX comment in the patch. And another thing to solve: I haven't included checks for the startTime property yet, because I think there's a bug in how it works in DownloadCore right now, so I don't know what's the right thing for DownloadImport to do (and for the test to check). Right now, when a download is started, it's startTime is set to a `new Date()` object. However, when we serialize that to disk, that becomes a string, and when we unserialize it again it remains a string. So it can be either an object or a String, depending if Firefox has restarted.. To add insult to injury, the previous DB stored them as timestamp integers, and DownloadImport doesn't do any conversion to them.. So besides being a string or an obj, it can also be an int if it was imported! :P

(In reply to :Felipe Gomes from comment #1) > after I import a download that will autoResume, I > need to wait for it to finish, but I've been unable to figure out how to do > so. I tried using the promiseDownloadStopped function but that it doesn't > work (it always returns a rejected promise).. In the meantime the > workaround I used to run the tests is that I added a `yield` in > DownloadImport so it will wait by itself for the download to finish Is it possible that the download is actually failing? In this case we would enter the "Error importing download: " case in DownloadImport, while the download is still added to the list. Looking at the exception reported by promiseDownloadStopped can also provide more hints as to what is happening here. > And another thing to solve: I haven't included checks for the startTime > property yet, because I think there's a bug in how it works in DownloadCore > right now, so I don't know what's the right thing for DownloadImport to do > (and for the test to check). > > Right now, when a download is started, it's startTime is set to a `new > Date()` object. However, when we serialize that to disk, that becomes a > string, and when we unserialize it again it remains a string. So it can be > either an object or a String, depending if Firefox has restarted.. > > To add insult to injury, the previous DB stored them as timestamp integers, > and DownloadImport doesn't do any conversion to them.. So besides being a > string or an obj, it can also be an int if it was imported! :P Hey, writing regression tests really helps! This is definitely a bug due to JSON not supporting Date objects natively. We can fix it while we're here. So, the correct state is to have a Date object in Download, and a string in the JSON file, using the format used by our toJSON method of Date instances (that is automatically called on serialization). We should change the deserialization to do "new Date(serialized.startTime)" to parse the string, and import to do "new Date(databaseStartTime / 1000)" (the PRTime type uses microseconds).

Comment on attachment 801064 [details] [diff] [review] testimport Review of attachment 801064 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/test/unit/test_DownloadImport.js @@ +88,5 @@ > + > +function promiseTableCount(aConnection) { > + return aConnection.execute("SELECT COUNT(*) FROM moz_downloads") > + .then(function(res) res[0].getResultByName("COUNT(*)"), > + Cu.reportError); I didn't do a full review yet, but I noticed this should be... .then(function(res) res[0].getResultByName("COUNT(*)")) .then(null, Cu.reportError); ...in case getResultByName throws an exception. @@ +162,5 @@ > + startTime: 1, > + state: DOWNLOAD_PAUSED, > + referrer: httpUrl("referrer1"), > + // XXX Paolo: I dont know how to generate the expected entityID > + entityID: "/31/Tue, 05 Feb 2013 17:49:18 GMT", In general, the EntityID can be any string, and if the server doesn't provide one then our code builds one using the Content-Length and other attributes. If you're trying to resume the download, this should match the expectations of "httpd.js", that I don't know exactly. Maybe, for resuming to work, you need to download from the "interruptible_resumable.txt" source file. @@ +269,5 @@ > + // preferredApplication. It's possibly a bug in the importer > + // or the Download.fromSerialized function. Granted, this situation > + // will not occur in a real database. > + preferredAction: Ci.nsIMIMEInfo.useSystemDefault, > + preferredApplication: "prerredApplication5", Yes, we can safely treat this case like we do currently for other invalid cases, considering the value as either useHelperApp or saveToDisk.

(In reply to :Paolo Amadini from comment #2) > Is it possible that the download is actually failing? In this case we would > enter the "Error importing download: " case in DownloadImport, while the > download is still added to the list. > > Looking at the exception reported by promiseDownloadStopped can also provide > more hints as to what is happening here. The download was working properly, and source.txt works fine (no need to use the interruptible one). But I figured it out: since DownloadImport calls download.start(), the download was in progress, so it wasn't either stopped, nor finished. And promiseDownloadStopped couldn't handle that case.. So I just wrote my own function to properly wait (and I also needed to handle the cases where the download is not gonna be resumed). > @@ +162,5 @@ > > + startTime: 1, > > + state: DOWNLOAD_PAUSED, > > + referrer: httpUrl("referrer1"), > > + // XXX Paolo: I dont know how to generate the expected entityID > > + entityID: "/31/Tue, 05 Feb 2013 17:49:18 GMT", > > In general, the EntityID can be any string, and if the server doesn't > provide one then our code builds one using the Content-Length and other > attributes. > > If you're trying to resume the download, this should match the expectations > of "httpd.js", that I don't know exactly. Maybe, for resuming to work, you > need to download from the "interruptible_resumable.txt" source file. Ok, so instead of forcing an entityID through the server or trying to hard-code the rules, I'm just opening a channel once at the beginning to figure out what's the entityID and use it through the test.

the autoResume == 1 change in DownloadImport.jsm is not really necessary but it's added for correctness (since it's an int stored in the DB)

(In reply to :Paolo Amadini from comment #2) > Hey, writing regression tests really helps! This is definitely a bug due to > JSON not supporting Date objects natively. We can fix it while we're here. > > So, the correct state is to have a Date object in Download, and a string in > the JSON file, using the format used by our toJSON method of Date instances > (that is automatically called on serialization). > > We should change the deserialization to do "new Date(serialized.startTime)" > to parse the string, and import to do "new Date(databaseStartTime / 1000)" > (the PRTime type uses microseconds). Done. I'm posting it here in this bug, but if you want me to post it to a separate one let me know

Comment on attachment 801168 [details] [diff] [review] Fix startTime serialization Review of attachment 801168 [details] [diff] [review]: ----------------------------------------------------------------- It's fine to fix this as part of this bug. I've a few minor comments but looks good overall! ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +811,5 @@ > serializable.error = { message: this.error.message }; > } > > + if (this.startTime) { > + serializable.startTime = this.startTime; nit: we may call this.startTime.toJSON() ourselves so we get the string in the in-memory representation also. It's more consistent in case the serializable representation is used without a JSON round-trip. ::: toolkit/components/jsdownloads/test/unit/test_Downloads.js @@ +171,5 @@ > + * Tests that the serialization/deserialization of the startTime Date > + * object works correctly. > + */ > + add_task(function test_startTimeSerialization() > + { Fix indentation. Also, this should be a "test_toSerializable_startTime" in "test_DownloadCore.js", or, even better, in "common_test_Download.js" using promiseStartDownload/promiseDownloadStopped (so we check the serialization in the legacy download case also). @@ +184,5 @@ > + > + let download2 = yield Downloads.createDownload(reserialized); > + > + do_check_eq(download1.startTime.constructor.name, "Date"); > + do_check_eq(download2.startTime.constructor.name, "Date"); For completeness, also check that the two dates are equal.

Comment on attachment 801167 [details] [diff] [review] test_DownloadImport Review of attachment 801167 [details] [diff] [review]: ----------------------------------------------------------------- This is the right general direction, since it's an entire new test module it needs some more work to be completed. I'll do another more detailed review pass after this one, on a patch where the various general issues have been fixed. In general, it would help to distinguish between variables containing Download objects and variables with database rows, calling them "download" and "downloadRow" respectively in a consistent way throughout the test file. Also, please add brief JavaDoc-style comment to the support functions, with more details for the less obvious ones. ::: toolkit/components/jsdownloads/test/unit/test_DownloadImport.js @@ +10,5 @@ > +//////////////////////////////////////////////////////////////////////////////// > +//// Globals > + > +Cu.import("resource://gre/modules/Sqlite.jsm"); > +Cu.import("resource://gre/modules/DownloadImport.jsm"); Please use lazy getters in tests also. @@ +89,5 @@ > + > +function promiseTableCount(aConnection) { > + return aConnection.execute("SELECT COUNT(*) FROM moz_downloads") > + .then(function(res) res[0].getResultByName("COUNT(*)"), > + Cu.reportError) This first Cu.reportError is unnecessary, if omitted the rejection will just fall through to the next rejection handler. nit: you may use an arrow function. @@ +104,5 @@ > + entityID = aRequest.entityID; > + } > + aRequest.cancel(Cr.NS_BINDING_ABORTED); > + }, > + onStopRequest: function(a,b,aStatusCode) { nit: use the proper names of the first two parameters. @@ +112,5 @@ > + deferred.reject("Unexpected status code received"); > + } > + }, > + onDataAvailable: function() { > + } mini-nit: "function () {}" with space and on one line. @@ +119,5 @@ > +} > + > +function getDownloadTarget(aLeafName) { > + let targetDownload = getTempFile(aLeafName); > + return Services.io.newFileURI(targetDownload).spec; return NetUtil.newURI(getTempFile(aLeafName)).spec; @@ +126,5 @@ > +function getPartialFile(aLeafName, aTainted = false) { > + let tempDownload = getTempFile(aLeafName); > + let partialContent = aTainted > + ? TEST_DATA_TAINTED.substr(0, 10) > + : TEST_DATA_SHORT.substr(0, 10); intentation nit: aTainted ? TEST_DATA_TAINTED.substr(0, 10) : TEST_DATA_SHORT.substr(0, 10); Also, using a constant for the number "10" may be useful since I get this is the same value used in test data later. @@ +131,5 @@ > + > + return OS.File.writeAtomic(tempDownload.path, > + partialContent, > + { bytes: partialContent.length }) > + .then(function() Promise.resolve(tempDownload.path)); .then(() => tempDownload.path) should be sufficient. @@ +151,5 @@ > + } > + > + let deferred = Promise.defer(); > + aDownload.onchange = function() { > + if (aDownload.stopped && aDownload.progress == 100) { This function is unneeded (see below), in any case you should never use the "progress" property as part of determining the download state (you should check "canceled", "succeeded" and "error" instead). @@ +161,5 @@ > + > + return deferred.promise; > +} > + > +function checkDownload(imported, original) { checkDownload(aDownload, aDownloadRow) @@ +166,5 @@ > + return Task.spawn(function() { > + do_check_eq(imported.source.url, original.source); > + do_check_eq(imported.source.referrer, original.referrer); > + > + do_check_eq(imported.target.path, NetUtil.newURI(original.target).QueryInterface(Ci.nsIFileURL).file.path); indentation: please stay within 80 characters. @@ +173,5 @@ > + do_check_eq(imported.stopped, true); > + > + // If expectedProgress = 100 means that the download will autoResume, > + // and with that a new startTime will be set. > + if (original.expectedProgress == 100) { It seems like "expectedProgress" isn't what we're trying to express here... a different boolean value might work better. @@ +229,5 @@ > + state: DOWNLOAD_PAUSED, > + referrer: httpUrl("referrer1"), > + entityID: sourceEntityId, > + maxBytes: 20, > + mimeType: "mimeType1", I think the "invalid MIME type" case should be the exception. @@ +237,5 @@ > + > + // Even though the information stored in the DB said > + // maxBytes was 20, the download turned out to be 31 bytes long. > + expectedBytes: 31, > + expectedMaxBytes: 31, Use the available constants for all these values. For the "expected" keys, use the names of the properties on the Download object, for example expectedCurrentBytes. @@ +290,5 @@ > + expectedBytes: 10, > + expectedMaxBytes: 20, > + expectedProgress: 50, > + expectedMimeType: "mimeType3", > + expectedContent: "This test ", Also here, obtain the expectedContent from the first part of the available string constant. @@ +554,5 @@ > + do_check_eq((yield promiseTableCount(connection)), > + gDownloadsToImport.length + gDownloadsNonImportable.length); > + > + // Close the connection so that DownloadImport can open it > + yield connection.close(); nit: close the connection in a finally block @@ +565,5 @@ > + do_check_eq(items.length, gDownloadsToImport.length); > + > + let checked = 0; > + for (let i = 0; i < gDownloadsToImport.length; i++) { > + yield promiseWaitDownload(items[i], gDownloadsToImport[i]); I don't think you need this function. Since you know (from the test data) whether a download to import is expected to be started, you can just call promiseDownloadStopped only on those downloads. @@ +571,5 @@ > + yield checkDownload(items[i], gDownloadsToImport[i]); > + checked++; > + } > + > + do_check_eq(checked, gDownloadsToImport.length); This check can be removed.

Thanks, all the tips worked. Here's an updated patch with most of the comments fixed (In reply to :Paolo Amadini from comment #8) > @@ +126,5 @@ > > +function getPartialFile(aLeafName, aTainted = false) { > > + let tempDownload = getTempFile(aLeafName); > > + let partialContent = aTainted > > + ? TEST_DATA_TAINTED.substr(0, 10) > > + : TEST_DATA_SHORT.substr(0, 10); > > intentation nit: aTainted ? TEST_DATA_TAINTED.substr(0, 10) > : TEST_DATA_SHORT.substr(0, 10); > I aligned the =?: but couldn't do as you mentioned because with another change made to this line it would go over 80 columns > @@ +229,5 @@ > > + state: DOWNLOAD_PAUSED, > > + referrer: httpUrl("referrer1"), > > + entityID: sourceEntityId, > > + maxBytes: 20, > > + mimeType: "mimeType1", > > I think the "invalid MIME type" case should be the exception. > I didn't understand this comment

Can you check if this is what you expected from the requested changes?

Comment on attachment 801853 [details] [diff] [review] test_DownloadImport Review of attachment 801853 [details] [diff] [review]: ----------------------------------------------------------------- This looks excellent, and the detailed comments make it very easy to follow the logic! (In reply to :Felipe Gomes from comment #9) > > @@ +229,5 @@ > > > + state: DOWNLOAD_PAUSED, > > > + referrer: httpUrl("referrer1"), > > > + entityID: sourceEntityId, > > > + maxBytes: 20, > > > + mimeType: "mimeType1", > > > > I think the "invalid MIME type" case should be the exception. > > > I didn't understand this comment I meant that, in a typical database that we are going to import, the contents of the MIME type field will look like "text/plain", "application/octet-stream", or something similar, and our tests should work on typical data. Some tests may be meant to work on invalid or atypical data, but they are the exception, as they are individually designed to test special cases. ::: toolkit/components/jsdownloads/test/unit/test_DownloadImport.js @@ +43,5 @@ > +// ongoing download. > +const TEST_DATA_PARTIAL_LENGTH = TEST_DATA_REPLACEMENT.length; > +// The value of the "maxBytes" column stored in the DB about the downloads. > +// It's intentionally different than TEST_DATA_LENGTH to test that each value > +// is seem when expected. "is seen"? nit: separate with a blank before each comment block, for readability. @@ +225,5 @@ > + > + return OS.File.writeAtomic(tempDownload.path, > + partialContent, > + { bytes: TEST_DATA_PARTIAL_LENGTH }) > + .then(() => tempDownload.path); Maybe the documentation of writeAtomic lags behind, but I've seen that "tmpPath" is mandatory, the data requires a TextEncoder, and I don't see any "bytes" option. Has this changed? @@ +281,5 @@ > + do_check_neq(aDownload.startTime.toJSON(), > + aDownloadRow.startTime.toJSON()); > + } else { > + do_check_false(aDownload.succeeded); > + do_check_neq(aDownload.progress, 100); While it doesn't happen with the test data, technically we may have a 100% progress for a download that has not finished yet, so I'd remove this neq check for better correctness. @@ +297,5 @@ > + } > + > + if (aDownloadRow.entityID) { > + do_check_eq(serializedSaver.entityID, aDownloadRow.entityID); > + } I think you can check the entityID property of the saver directly here, no need to check the serialized representation. @@ +305,5 @@ > + > + if (aDownload.progress > 0) { > + let fileToCheck = aDownload.progress == 100 > + ? aDownload.target.path > + : aDownload.target.partFilePath; You need to check expectedResume here, and the presence of expectedContent, and ignore the "progress" property. @@ +533,5 @@ > + */ > +add_task(function prepareNonImportableDownloads() > +{ > + gDownloadsRowNonImportable = [ > + // Download with no source "Download with no source (should never happen in normal circumstances)."

Updated summary and marked as tracking release for the important startTime serialization fix.

(In reply to :Paolo Amadini from comment #11) > Comment on attachment 801853 [details] [diff] [review] > test_DownloadImport > > Review of attachment 801853 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks excellent, and the detailed comments make it very easy to follow > the logic! > > (In reply to :Felipe Gomes from comment #9) > > > @@ +229,5 @@ > > > > + state: DOWNLOAD_PAUSED, > > > > + referrer: httpUrl("referrer1"), > > > > + entityID: sourceEntityId, > > > > + maxBytes: 20, > > > > + mimeType: "mimeType1", > > > > > > I think the "invalid MIME type" case should be the exception. > > > > > I didn't understand this comment > > I meant that, in a typical database that we are going to import, the > contents of the MIME type field will look like "text/plain", > "application/octet-stream", or something similar, and our tests should work > on typical data. Some tests may be meant to work on invalid or atypical > data, but they are the exception, as they are individually designed to test > special cases. Ah ok, got it! I left 3 cases to make sure it works as expected on resumed/non-resumed downloads, and changed the other to typical data (I kept them all as text/plain as the source file is source.txt) > > + return OS.File.writeAtomic(tempDownload.path, > > + partialContent, > > + { bytes: TEST_DATA_PARTIAL_LENGTH }) > > + .then(() => tempDownload.path); > > Maybe the documentation of writeAtomic lags behind, but I've seen that > "tmpPath" is mandatory, the data requires a TextEncoder, and I don't see any > "bytes" option. Has this changed? > Yeah, I think it's a bit outdated from when write was removed.. I'll get in touch with Yoric to see if I can rely on all these things not changing.. But I looked at the code to writeAtomic and it all works as expected with these given parameters

https://hg.mozilla.org/integration/fx-team/rev/6a5406d79364 https://hg.mozilla.org/integration/fx-team/rev/28df44ca24f3

Backed out in https://hg.mozilla.org/integration/fx-team/rev/dd627cf60b9e for mochitest-bc bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=27677755&tree=Fx-Team

So what happened is that when we do `new Date(obj)`, where obj is a Date, turns out that the milliseconds info is not preserved! browser_basic_functionality generates 4 download entries, each 1ms apart, and expects their ordering in the panel (sorted by date) to be correct.

Comment on attachment 803180 [details] [diff] [review] Fix startTime serialization (In reply to :Felipe Gomes from comment #16) > So what happened is that when we do `new Date(obj)`, where obj is a Date, > turns out that the milliseconds info is not preserved! Wow, absolutely counter-intuitive! The fix looks good to me.

https://hg.mozilla.org/integration/fx-team/rev/c6d7c7739056 https://hg.mozilla.org/integration/fx-team/rev/270d7ca88c31

https://hg.mozilla.org/mozilla-central/rev/c6d7c7739056 https://hg.mozilla.org/mozilla-central/rev/270d7ca88c31

Backed out for high intermittent-failure rate: https://tbpl.mozilla.org/php/getParsedLog.php?id=27767079&tree=Mozilla-Central https://tbpl.mozilla.org/php/getParsedLog.php?id=27767561&tree=Mozilla-Central https://tbpl.mozilla.org/php/getParsedLog.php?id=27762787&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=27767806&tree=Mozilla-Inbound remote: https://hg.mozilla.org/mozilla-central/rev/bea6fbdc8d6c remote: https://hg.mozilla.org/mozilla-central/rev/7f28b6fac8bd

(Note the Windows failures were all PGO)

Felipe, any idea about what may be causing this?

It appears that OS.File.writeAtomic is not finishing before promiseVerifyContents runs. I added the tmpPath to make it a "real" writeAtomic, and also added flush: true for good measure. I was hoping I wouldn't need the .tmp file move in the test, but without it it appears to be unreliable. at least, that's the theory.. let's see what try says. Try run with PGO enabled: https://tbpl.mozilla.org/?tree=Try&rev=aaf55109bb43 No idea about the crash on mac, but since it happened in the same testcase, i'm willing to believe it's the same issue manifested differently.

Not sure what happened with the previous push, seems like it hit a disk space clean-up.. Pushed again with an updated tree: https://tbpl.mozilla.org/?tree=Try&rev=09857a57b573

The test is still having issues so I moved the startTime part to bug 915968 in order to land it and unblock 906139, and on this bug I'll continue to look into what's going on with this intermittent test problem.

An update: The two issues that were seem when this landed turned out to be the existing bug 915214 and bug 901017 intermittent failures, which happened more often in this test (at least for PGO builds). We've been investigating how to fix them and will re-land this test once we figure it out.

This performed great on top of its two dependencies: https://tbpl.mozilla.org/?tree=Try&rev=ce27bf80cec1

Also green on a PGO build: https://tbpl.mozilla.org/?tree=Try&rev=828eea11faeb

The DownloadImport module handles the migration of in-progress downloads from the old to the new back-end. This bug contains automated tests for it, and landing was delayed due to the intermittent failures in bug 915214. Now that we can land this module, I think we should also uplift it to Aurora for stability, and avoid regressions later in case we make changes in Beta.

Comment on attachment 809765 [details] [diff] [review] Rebased patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 825588 User impact if declined: More risk of regressions later (comment 29) Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Low (test only) String or IDL/UUID changes made by this patch: None

Comment on attachment 809765 [details] [diff] [review] Rebased patch The DownloadImport.jsm change looks not test-only? I assume it's safe, though.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #33) > The DownloadImport.jsm change looks not test-only? I assume it's safe, though. Ah, missed that! It should be safe but I may also just remove it from the Aurora patch.

Don't need to track test-only, it can be approved for uplift since it's low risk (assuming you are removing the non-test-only part from the Aurora patch).

Landed on Aurora without the change in the module: https://hg.mozilla.org/releases/mozilla-aurora/rev/2055faffc05d