932803 - [Contacts API] Add tests for migrations

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[Julien Wajsberg [:julienw]]

Attachment: WIP patch v1

We should test migrations on problematic and good contacts.

The attached patch is an experiment using a XPCshell test, but now I think we need to access the real navigator.mozContacts API so that we know if WebIDL fails.

So I think I'll convert this to mochitest instead, or maybe use both.

Comment 1

[Reuben Morais [:reuben]]

You should be able to extend test_contacts_upgrade.html to also check with known bad contacts. Also, this is way cleaner than the mochitest-chrome setup there, so I'll convert that test to a xpcshell test if you don't :)

Comment 2

[Julien Wajsberg [:julienw]]

Problem with xpcshell is that you can't access mozContact :( and a lot of our latest problems came from the WebIDL bindings, and therefore from mozContact.

I'm considering using a mochitest-plain, but using ochameau's loadChromeScript [1] to initialize the indexedDB before :)

[1] http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#497

Comment 3

[Dietrich Ayala (:dietrich)]

Would love to see this exist :D Any idea when you can finish it?

Comment 4

[Julien Wajsberg [:julienw]]

This is (sadly) quite low priority for now, because other more critical bugs are still piling up...

Comment 5

[Reuben Morais [:reuben]]

Can you not accomplish this by extending test_contacts_upgrade.xul?

Comment 6

[Julien Wajsberg [:julienw]]

I'd like to do a mochitest-plain instead of a mochitest-chrome. But still the longest work is not to know where to do this, but to first create the old db (part of this is in the current patch), and to do the various checks ;)

My goal for the start of January is to finishing up most of my current bugs instead of taking new ones so hopefully I'll be able to finish this up. That said, Reuben, if you have some bandwidth, feel free to take over!

Comment 7

[Reuben Morais [:reuben]]

So I looked more closely into this. Writing tests for individual upgrade paths is very tricky because our code assumes lots of things (naturally) about the DB layout, and we get lots of failures when the DB doesn’t match it. So for example you save a contact in revision 0, then starts CDB with version 1 and invalidateCache or incrementRevision will fail, because those stores don't exist.

We'd need the ContactDB code at the time of the DB version in order to properly test it, or we'd need to make operations fail sanely when stores and indexes are missing. I'm not sure if the latter is worth the code complexity and developer time.

Comment 8

[Julien Wajsberg [:julienw]]

Yep, I think the only thing we can do is:
* generate a db in version XXX we choose to test something
* upgrade to the current version and test

We can't test the db in a specific version. But this is already good enough IMO!

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Reuben, is it enough tests for now?

Try is: https://tbpl.mozilla.org/?tree=Try&rev=8bbf1bd07436

Comment 10

[Julien Wajsberg [:julienw]]

Here is the new try after rebasing: https://tbpl.mozilla.org/?tree=Try&rev=ad2c7a205056

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8370246 [details] [diff] [review]
patch v2

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

::: dom/contacts/tests/migration/test_migration.html
@@ +87,5 @@
> +
> +    req.onerror = function onerror() {
> +      ok(false, "Couldn't access the mozContacts API");
> +      next();
> +    };

In the future, I'd like to check that all contacts look fine. That said, "find" should already try to convert all of them using WebIDL so we're already quite safe.

::: dom/contacts/tests/migration/test_migration_chrome.js
@@ +1,4 @@
> +/* global
> +    sendAsyncMessage,
> +    addMessageListener
> + */

These are instructions for jshint that I configured locally.

@@ +31,5 @@
> +      db.createObjectStore(SAVED_GETALL_STORE_NAME);
> +      db.createObjectStore(REVISION_STORE).put(0, "revision");
> +    },
> +    BAD: [
> +      {

Reading this now, I think it needs a comment explaining why this contact is bad. I think it's the "name" property that is not an array here.

Comment 12

[Julien Wajsberg [:julienw]]

1.3+ bug 951806 depends on this to be less risky -> requesting 1.3+ here as well.

Comment 13

[Jason Smith [:jsmith]]

Blocks a blocker. We aren't landing the blocking bug here without this done.

Comment 14

[Julien Wajsberg [:julienw]]

I get
  ERROR http://mochi.test:8888/tests/dom/contacts/tests/migration/test_migration_chrome.js:85 - TypeError: indexedDB is undefined
in B2G builds.

Back to work ;)

Comment 15

[Reuben Morais [:reuben]]

Comment on attachment 8370246 [details] [diff] [review]
patch v2

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

I still don't know why you go through all this trouble to use mochitest-plain instead of extending the existing chrome test, other than because "I'd like to" in comment 6, but a test is a test.

::: dom/contacts/tests/migration/test_migration_chrome.js
@@ +31,5 @@
> +      db.createObjectStore(SAVED_GETALL_STORE_NAME);
> +      db.createObjectStore(REVISION_STORE).put(0, "revision");
> +    },
> +    BAD: [
> +      {

Yep.

Comment 16

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #15)
> Yep.

This was in reply to your comment that you should explain why that contact is bad.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #16)
> (In reply to Reuben Morais [:reuben] from comment #15)
> > Yep.
> 
> This was in reply to your comment that you should explain why that contact
> is bad.

I got it ;)

(In reply to Reuben Morais [:reuben] from comment #15)
> Comment on attachment 8370246 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8370246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still don't know why you go through all this trouble to use
> mochitest-plain instead of extending the existing chrome test, other than
> because "I'd like to" in comment 6, but a test is a test.

Mainly because it's closer to what the apps are using. But maybe I'm wrong.

Comment 18

[Julien Wajsberg [:julienw]]

Spent some time to struggle with running the tests on either B2G Desktop or the Emulator today, so I couldn't move forward as much as I wanted => moving the ETA.

Comment 19

[Julien Wajsberg [:julienw]]

I'm probably stupid, but I can't make Cu.import to import correctly the various exported objects to my chrome script in B2G environments. It works fine when running in a Firefox environment. I tried putting "this"

I could import indexedDB using |Cu.importGlobalProperties(["indexedDB"]);| but that's all.

Therefore, I think we should skip the test in non-Firefox environments. When it's run in Firefox it already tests the upgrade steps so I think it's fine.

I can also try to convert it to a mochitest_chrome.

Comment 20

[Julien Wajsberg [:julienw]]

Correction: importing "Promise" works fine, but not the other ones.
When I put an object scope as second argument, and then I try to display it using both Object.keys and JSON.stringify, I see that the actual keys are here, but that the values are undefined. Except for Promise then.

I don't understand what makes the "Promise" JSM different.

Comment 21

[Julien Wajsberg [:julienw]]

Alexandre helped me so I think I'm not blocked anymore !

Comment 22

[Julien Wajsberg [:julienw]]

Attachment: patch v3

New try is https://tbpl.mozilla.org/?tree=Try&rev=333b68f671c1

You need the patch to bug 968857 to be able to run these tests, because the loadChromeScript utility had a bug with too big files.

After applying the patch to bug 968857, you need to run "./mach build testing/specialpowers/" so that the new version is correctly used.

Also, this patch applies after the patches in bug 951806, because it tests cases that are fixed in the first patch there.

So I added a bunch of tests covering most of the upgrade paths.

Also I found that using mochitest-plain instead of mochitest-chrome makes it possible to run them in the emulator.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8371663 [details] [diff] [review]
patch v3

These tests trigger an issue in another test. I'm digging it right now so moving from review to feedback.

Especially I'll also change how the substring matching is configured.

Comment 24

[Julien Wajsberg [:julienw]]

Attachment: patch v4

Here are the changes compared to the previous patch:

* I fixed how the substringmatching pref are reset in each tests. If we don't reset the cached Mcc in the pref, then the substring matching boolean is not configured again
* I now use the pref to enable/disable substring matching instead of using ContactService directly.
* I delete the DB before creating it, to prevent any edge effect

You need to have the first patch of Bug 951806 (the name index fix) but the nice thing is that it passes with or without the second patch.

New try is https://tbpl.mozilla.org/?tree=Try&rev=60858397111d

Comment 25

[Julien Wajsberg [:julienw]]

Try is quite green ;)

Comment 26

[Reuben Morais [:reuben]]

Comment on attachment 8372375 [details] [diff] [review]
patch v4

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

r=me

::: dom/contacts/fallback/ContactDB.jsm
@@ +19,5 @@
>  Cu.import("resource://gre/modules/IndexedDBHelper.jsm");
>  Cu.import("resource://gre/modules/PhoneNumberUtils.jsm");
>  Cu.importGlobalProperties(["indexedDB"]);
>  
> +/* all exported symbols needs to be bound to this on B2G - Bug 961777 */

need

::: dom/contacts/fallback/ContactService.jsm
@@ +22,5 @@
>                                     "@mozilla.org/parentprocessmessagemanager;1",
>                                     "nsIMessageListenerManager");
>  
> +
> +/* all exported symbols needs to be bound to this on B2G - Bug 961777 */

need

::: dom/contacts/tests/migration/mochitest.ini
@@ +1,2 @@
> +[DEFAULT]
> +[test_migration.html]

Skip this on Android.

::: dom/contacts/tests/migration/test_migration.html
@@ +71,5 @@
> +}
> +
> +var steps = [
> +  function setupChromeScript() {
> +    info("Creating the database at version 14");

This log is outdated.

@@ +97,5 @@
> +    };
> +  },
> +
> +  function testRetrieveAllContacts() {
> +    info("Checking the contacts are corrected to obey WebIDL constraints.  (upgrades 14 to 17)");

If one of upgrades 14-17 fails or doesn't run, we'll throw while converting the DB object to a mozContact, which is not detectable by the test. So the failure we want to detect with this test will manifest in the form of a timeout. Can you add a comment explaining that?

@@ +140,5 @@
> +      }
> +
> +      var req = mozContacts.find({
> +        filterBy: ["name"],
> +        filterValue: [name],

s/[name]/name/, filterValue is a DOMString

@@ +187,5 @@
> +
> +    req.onerror = function onerror() {
> +      ok(false, "Error while finding contact for substring matching check!");
> +      next();
> +    }

nit: semicolon

::: dom/contacts/tests/migration/test_migration_chrome.js
@@ +23,5 @@
> +  Promise
> +} = imports;
> +
> +function debug(str) {
> +  dump("-*- TestMigrationChromeScript: " + str + "\n");

This should be disabled by default.

@@ +27,5 @@
> +  dump("-*- TestMigrationChromeScript: " + str + "\n");
> +}
> +
> +const DATA = {
> +  12: {

Neat!

::: dom/contacts/tests/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +MOCHITEST_MANIFESTS += ['migration/mochitest.ini', 'mochitest.ini']

I'd rather keep this in tests/.

Comment 27

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #26)

Will fix the nits but I have one comment and one question here:

> 
> @@ +97,5 @@
> > +    };
> > +  },
> > +
> > +  function testRetrieveAllContacts() {
> > +    info("Checking the contacts are corrected to obey WebIDL constraints.  (upgrades 14 to 17)");
> 
> If one of upgrades 14-17 fails or doesn't run, we'll throw while converting
> the DB object to a mozContact, which is not detectable by the test. So the
> failure we want to detect with this test will manifest in the form of a
> timeout. Can you add a comment explaining that?

You're right, but there is something else that I want to check here: that we didn't mess up something in the upgrades, and that the contacts correctly convert to WebIDL, even if all the upgrade steps ran. This is an issue we had in the past, right?

> 
> @@ +140,5 @@
> > +      }
> > +
> > +      var req = mozContacts.find({
> > +        filterBy: ["name"],
> > +        filterValue: [name],
> 
> s/[name]/name/, filterValue is a DOMString

Mmm yep, but how could this work then? I'll figure this out.

> 
> ::: dom/contacts/tests/migration/test_migration_chrome.js
> @@ +23,5 @@
> > +  Promise
> > +} = imports;
> > +
> > +function debug(str) {
> > +  dump("-*- TestMigrationChromeScript: " + str + "\n");
> 
> This should be disabled by default.

You mean, the logs ?
I was thinking that since we're in the tests, this is a good thing to always have logs in case it's failing. Do you think it's better to keep them shut and only enable them when we need it?

Comment 28

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #27)
> > @@ +97,5 @@
> > > +    };
> > > +  },
> > > +
> > > +  function testRetrieveAllContacts() {
> > > +    info("Checking the contacts are corrected to obey WebIDL constraints.  (upgrades 14 to 17)");
> > 
> > If one of upgrades 14-17 fails or doesn't run, we'll throw while converting
> > the DB object to a mozContact, which is not detectable by the test. So the
> > failure we want to detect with this test will manifest in the form of a
> > timeout. Can you add a comment explaining that?
> 
> You're right, but there is something else that I want to check here: that we
> didn't mess up something in the upgrades, and that the contacts correctly
> convert to WebIDL, even if all the upgrade steps ran. This is an issue we
> had in the past, right?

Sure. Just make sure the timeout is also documented since that's the main thing it's testing and there's nothing in the code indicating which part of the test catches the failure.

> > @@ +140,5 @@
> > > +      }
> > > +
> > > +      var req = mozContacts.find({
> > > +        filterBy: ["name"],
> > > +        filterValue: [name],
> > 
> > s/[name]/name/, filterValue is a DOMString
> 
> Mmm yep, but how could this work then? I'll figure this out.

JS shenanigans. |["foo"].toString() == "foo"|

> > ::: dom/contacts/tests/migration/test_migration_chrome.js
> > @@ +23,5 @@
> > > +  Promise
> > > +} = imports;
> > > +
> > > +function debug(str) {
> > > +  dump("-*- TestMigrationChromeScript: " + str + "\n");
> > 
> > This should be disabled by default.
> 
> You mean, the logs ?
> I was thinking that since we're in the tests, this is a good thing to always
> have logs in case it's failing. Do you think it's better to keep them shut
> and only enable them when we need it?

Yes. We don't want to inflate log sizes unnecessarily, and for things that are always helpful (not just when it's failing) you can use info().

Comment 29

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #28)
> (In reply to Julien Wajsberg [:julienw] from comment #27)
> > > @@ +97,5 @@
> > > > +    };
> > > > +  },
> > > > +
> > > > +  function testRetrieveAllContacts() {
> > > > +    info("Checking the contacts are corrected to obey WebIDL constraints.  (upgrades 14 to 17)");
> > > 
> > > If one of upgrades 14-17 fails or doesn't run, we'll throw while converting
> > > the DB object to a mozContact, which is not detectable by the test. So the
> > > failure we want to detect with this test will manifest in the form of a
> > > timeout. Can you add a comment explaining that?
> > 
> > You're right, but there is something else that I want to check here: that we
> > didn't mess up something in the upgrades, and that the contacts correctly
> > convert to WebIDL, even if all the upgrade steps ran. This is an issue we
> > had in the past, right?
> 
> Sure. Just make sure the timeout is also documented since that's the main
> thing it's testing and there's nothing in the code indicating which part of
> the test catches the failure.


Ok, I think I understand now. WebIDL errors do not trigger the "onerror" callbacks, which is quite an issue IMO, but it's out of our hands I guess.

> 
> > > @@ +140,5 @@
> > > > +      }
> > > > +
> > > > +      var req = mozContacts.find({
> > > > +        filterBy: ["name"],
> > > > +        filterValue: [name],
> > > 
> > > s/[name]/name/, filterValue is a DOMString
> > 
> > Mmm yep, but how could this work then? I'll figure this out.
> 
> JS shenanigans. |["foo"].toString() == "foo"|

["JS is awesome, "right?"].toString()

> 
> > > ::: dom/contacts/tests/migration/test_migration_chrome.js
> > > @@ +23,5 @@
> > > > +  Promise
> > > > +} = imports;
> > > > +
> > > > +function debug(str) {
> > > > +  dump("-*- TestMigrationChromeScript: " + str + "\n");
> > > 
> > > This should be disabled by default.
> > 
> > You mean, the logs ?
> > I was thinking that since we're in the tests, this is a good thing to always
> > have logs in case it's failing. Do you think it's better to keep them shut
> > and only enable them when we need it?
> 
> Yes. We don't want to inflate log sizes unnecessarily, and for things that
> are always helpful (not just when it's failing) you can use info().

I con't use "info" in the chrome script but ok, I've disabled this by default.

Comment 30

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #29)
> Ok, I think I understand now. WebIDL errors do not trigger the "onerror"
> callbacks, which is quite an issue IMO, but it's out of our hands I guess.

Not just WebIDL errors, but pretty much anything that throws in ContactManager. We have to trust the parent, if the data coming from it is broken there's not a lot the child can do.

Comment 31

[Julien Wajsberg [:julienw]]

Attachment: patch v5

Keeping r=reuben, fixed comments. 

new Try is https://tbpl.mozilla.org/?tree=Try&rev=086bf58cdfd2

Comment 32

[Julien Wajsberg [:julienw]]

Try is green => checkin-needed.

You need to land the patch 1. from bug 951806 before this one.

For v1.3 you also need to uplift bug 951806 first.

Comment 33

[Julien Wajsberg [:julienw]]

Sorry, for v1.3 you also need to uplift bug 968857 first.

Comment 34

[Julien Wajsberg [:julienw]]

Try for v1.3 is: https://tbpl.mozilla.org/?tree=Try&rev=1fb17ccac4fb

Comment 35

[Ryan VanderMeulen [:RyanVM]]

If trunk depends on bug 951806 landing first, let's hold off on requesting checkin until that's ready to land as well.

Comment 36

[Julien Wajsberg [:julienw]]

Ok, works for me.

Comment 37

[Julien Wajsberg [:julienw]]

Bug 951806 is ready ! Please land Bug 951806 first, and then this one.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/491fff0a0cda

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/491fff0a0cda

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a05520c3b5d8

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Backed out from v1.3 for test timeouts. Don't know yet if they affect emulator builds or just desktop builds.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/20eb02c378ce

https://tbpl.mozilla.org/php/getParsedLog.php?id=34515949&tree=Mozilla-B2g28-v1.3

Comment 42

[Ryan VanderMeulen [:RyanVM]]

The failures were on B2G desktop only. Note that I did leave bug 951806 in and it appears to be sticking OK.

Comment 43

[Julien Wajsberg [:julienw]]

yeah, deleting the DB could timeout if there are other users somehow.

Ryan, do you see such timeouts on central? And on 1.3, is it intermittent?

Comment 44

[Julien Wajsberg [:julienw]]

Note that I could live with the tests only in central. What do you think Gregor?

Comment 45

[Ryan VanderMeulen [:RyanVM]]

It was perma-fail on 1.3. And no failures on m-c that I'm aware of.

Comment 46

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #44)
> Note that I could live with the tests only in central. What do you think
> Gregor?

Are we sure its not a real problem? We have more development going on within the 1.3 branch (tarako) so we should try to land this.

Comment 47

[Julien Wajsberg [:julienw]]

yeah I'm quite sure it's not a real problem (for instance it doesn't reproduce on the emulator, only in B2G-Desktop, which will be quite painful for me since I didn't manage to run the tests on B2G Desktop yet :( ).

But I understand the Tarako-related comment so I'll work on this a little more.

Heh, I would have liked to have my debug statements enabled by default ;-)

Comment 48

[Julien Wajsberg [:julienw]]

try with more debug: https://tbpl.mozilla.org/?tree=Try&rev=95d78f1d4609

Comment 49

[Julien Wajsberg [:julienw]]

Attachment: v1.3 patch

In this branch specific patch, I just add the test file in the b2g desktop ignore list, because it looks like loadChromeScript fails.

I get this in the test log:

03:20:54     INFO -  System JS : ERROR chrome://specialpowers/content/SpecialPowersObserverAPI.js:162 - Error

Which points to this code:

    var scriptableStream = Cc["@mozilla.org/scriptableinputstream;1"]
                             .getService(Ci.nsIScriptableInputStream);

Since it seems to work fine on emulator in v1.3, and on all environments in central, I think it's enough to just disable the test in v1.3 for b2g desktop only, and assume the specific issue was fixed on central.


Try is https://tbpl.mozilla.org/?tree=Try&rev=a6caa04163f8

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: migration is not tested
Testing completed: yes
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 50

[Julien Wajsberg [:julienw]]

Fabrice: I ask approval only for the small bits in dom/contacts/fallback, not for the tests. I bind the various objects to "this" instead of using a const so that they're correctly exported in B2G environments.

Comment 51

[Julien Wajsberg [:julienw]]

Try is green!

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3fb843458d33