1236154 - Investigate writing a test for bug 1229076 (ESE database reading on Windows)

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

This would likely involve either including a compressed db in the tree or creating one in the test, as an uncompressed empty DB is several MB in size in my testing. :-(

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1236154 - add test for ESE migration code, r?MattN

Review commit: https://reviewboard.mozilla.org/r/34571/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34571/

Comment 2

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34571/#review31249

Technically this is a test for the bookmarks rather than the reading list, I guess. I can file a followup about the reading list stuff if you like, but testing bookmarks seemed more useful because the code is slightly more complex, having to deal with hierarchies instead of just a list of items.

::: browser/components/migration/ESEDBReader.jsm:291
(Diff revision 1)
> +      // Shouldn't try to call JetTerm if the following call fails.
> +      this._instanceCreated = false;
>        ESE.Init(this._instanceId.address());
> +      this._instanceCreated = true;

Note that this...

::: browser/components/migration/EdgeProfileMigrator.js:254
(Diff revision 1)
> -      Cu.reportError("Failed to check for table " + tableName + " in Edge database at " +
> +      Cu.reportError("Failed to check for table " + this.TABLE_NAME + " in Edge database at " +

... and this

are bugfixes for the error handling in the code, rather than test-related.

Specifically for the JetInit thing, note that while the documentation for JetInit ( https://msdn.microsoft.com/en-us/library/gg294068%28v=exchg.10%29.aspx ) does not have a note about this, the documentation for JetInit3 (!) states that:

https://msdn.microsoft.com/en-us/library/gg269296%28v=exchg.10%29.aspx

"Note that JetTerm should not be called if the JetInit3 function fails. However, JetTerm should still be called for all instances created by JetCreateInstance2 if JetInit3 was never called or if JetInit3 succeeds."

IME this applies for JetInit and JetCreateInstance as well. Also, yes, this is really inconsistent and annoying. "Call this function if you do A, or if you do A and B, but not if you do A and then try to do B but fail". Seriously, who came up with that. :-\

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8718433 [details]
MozReview Request: Bug 1236154 - add test for ESE migration code, r?MattN

https://reviewboard.mozilla.org/r/34571/#review32311

rs=me. Not going to lie and say I auditted all of the test-only ESE methods…

::: browser/components/migration/tests/unit/test_Edge_db_migration.js:5
(Diff revision 1)
> +var eseBackStage = Cu.import("resource:///modules/ESEDBReader.jsm");
> +var ESE = eseBackStage.ESE;
> +var KERNEL = eseBackStage.KERNEL;
> +var gLibs = eseBackStage.gLibs;
> +var COLUMN_TYPES = eseBackStage.COLUMN_TYPES;
> +var declareESEFunction = eseBackStage.declareESEFunction;
> +var loadLibraries = eseBackStage.loadLibraries;
> +
> +var gESEInstanceCounter = 1;

Nit: Not sure why you're using `var` here. I thought we still use `let` everywhere unless we can't.

::: browser/components/migration/tests/unit/test_Edge_db_migration.js:293
(Diff revision 1)
> +  tempFile.createUnique(tempFile.DIRECTORY_TYPE, 384 /* 0600 */);

You should use `0o600` to avoid the comment

::: browser/components/migration/tests/unit/test_Edge_db_migration.js:300
(Diff revision 1)
> +  logs.create(tempFile.DIRECTORY_TYPE, 384 /* 0600 */);

Ditto

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6670d69a9f51

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6670d69a9f51