Closed Bug 932803 Opened 9 years ago Closed 9 years ago

[Contacts API] Add tests for migrations

Categories

(Core Graveyard :: DOM: Contacts, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch WIP patch v1 (obsolete) — Splinter Review
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.
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 :)
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
Would love to see this exist :D Any idea when you can finish it?
This is (sadly) quite low priority for now, because other more critical bugs are still piling up...
Can you not accomplish this by extending test_contacts_upgrade.xul?
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!
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.
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!
Blocks: 951806
Attached patch patch v2 (obsolete) — Splinter Review
Reuben, is it enough tests for now?

Try is: https://tbpl.mozilla.org/?tree=Try&rev=8bbf1bd07436
Attachment #824663 - Attachment is obsolete: true
Attachment #8370246 - Flags: feedback?(reuben.bmo)
Here is the new try after rebasing: https://tbpl.mozilla.org/?tree=Try&rev=ad2c7a205056
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.
1.3+ bug 951806 depends on this to be less risky -> requesting 1.3+ here as well.
blocking-b2g: --- → 1.3?
Blocks a blocker. We aren't landing the blocking bug here without this done.
blocking-b2g: 1.3? → 1.3+
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 ;)
Whiteboard: [ETA:2/6]
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.
Attachment #8370246 - Flags: feedback?(reuben.bmo) → feedback+
(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.
(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.
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.
Whiteboard: [ETA:2/6] → [ETA:2/7]
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.
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.
Alexandre helped me so I think I'm not blocked anymore !
Depends on: 968857
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #8370246 - Attachment is obsolete: true
Attachment #8371663 - Flags: review?(reuben.bmo)
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.
Attachment #8371663 - Flags: review?(reuben.bmo) → feedback?(reuben.bmo)
Attached patch patch v4 (obsolete) — Splinter Review
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
Attachment #8371663 - Attachment is obsolete: true
Attachment #8371663 - Flags: feedback?(reuben.bmo)
Attachment #8372375 - Flags: review?(reuben.bmo)
Whiteboard: [ETA:2/7] → [ETA:2/10]
Try is quite green ;)
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/.
Attachment #8372375 - Flags: review?(reuben.bmo) → review+
(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?
(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().
(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.
(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.
Attached patch patch v5Splinter Review
Keeping r=reuben, fixed comments. 

new Try is https://tbpl.mozilla.org/?tree=Try&rev=086bf58cdfd2
Attachment #8372375 - Attachment is obsolete: true
Attachment #8373216 - Flags: review+
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.
Keywords: checkin-needed
Sorry, for v1.3 you also need to uplift bug 968857 first.
If trunk depends on bug 951806 landing first, let's hold off on requesting checkin until that's ready to land as well.
Keywords: checkin-needed
Ok, works for me.
Whiteboard: [ETA:2/10] → [ETA:2/11]
Bug 951806 is ready ! Please land Bug 951806 first, and then this one.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/491fff0a0cda
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [ETA:2/11]
Target Milestone: --- → mozilla30
The failures were on B2G desktop only. Note that I did leave bug 951806 in and it appears to be sticking OK.
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?
Note that I could live with the tests only in central. What do you think Gregor?
Flags: needinfo?(anygregor)
It was perma-fail on 1.3. And no failures on m-c that I'm aware of.
(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.
Flags: needinfo?(anygregor)
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 ;-)
Flags: needinfo?(felash)
Attached patch v1.3 patchSplinter Review
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
Attachment #8375462 - Flags: review+
Attachment #8375462 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(felash) → needinfo?(fabrice)
Attachment #8375462 - Attachment is patch: true
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.
Try is green!
Attachment #8375462 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Flags: needinfo?(fabrice)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.