Test webapp runtime update service

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

Trunk
Firefox 33

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch Patch (obsolete) — Splinter Review
This is going to test the webapprt update service.

The test I've built installs an app, modifies the update interval to be 1 second and checks if the webapp runtime does a request to get the manifest and the new package (if it does, then this means it has called checkForUpdate, download, etc.).

I wanted to test that the update was actually prepared, but I couldn't find a safe way to do it.
Attachment #8453460 - Flags: feedback?(myk)
Posted patch Patch (obsolete) — Splinter Review
I forgot to add the manifest.webapp file to the patch.
Assignee: nobody → mar.castelluccio
Attachment #8453460 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8453460 - Flags: feedback?(myk)
Attachment #8453491 - Flags: feedback?(myk)
Comment on attachment 8453491 [details] [diff] [review]
Patch

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

::: toolkit/webapps/tests/app.sjs
@@ +113,5 @@
> +  }
> +
> +  if ("getPackageQueries" in query) {
> +    response.setHeader("Content-Type", "text/plain", false);
> +    response.write(String(Number(getState("getPackageQueries"))));

Why convert it to a number only to convert it back to a string?  It should already be the appropriate string, given the code that sets it above.

(Also, response.write will probably stringify a number correctly anyway, so even if we needed to convert it to a number, we probably wouldn't need to convert it back to a string.)

::: toolkit/webapps/tests/test_packaged_update_service.xul
@@ +94,5 @@
> +  let profileDir = OS.Path.join(testAppInfo.installPath, profileRelPath);
> +
> +  // Force the update timer to fire in 1 second
> +  let prefsFile = OS.Path.join(profileDir, "prefs.js");
> +  yield writeToFile(prefsFile, "user_pref(\"webapprt.app_update_interval\", 1);");

Given this interval, which is the same amount of time we wait while PackageQueries or ManifestQueries don't have their expected values (or the app hasn't been closed yet), could another update request race the first one while we're running the test?

Perhaps a better approach would be to move the updateTimer callback to a public method of the WebappRT object:

this.WebappRT = {
  …
  startUpdateService: function() {
    …
    timerManager.registerTimer("updateTimer", this.checkForUpdate.bind(this), 24 * 60 * 60);
  },
  
  checkForUpdate: function() {
    …
  },
};

Then call it directly from the test:

  WebappRT.checkForUpdate();

That wouldn't ensure that the timer manager correctly calls the callback; but that seems like it should be the responsibility of a timer manager test anyway.  Whereas our concern here is making sure our callback behaves as expected.

::: webapprt/WebappRT.jsm
@@ +94,5 @@
> +      try {
> +        this._updateInterval = Services.prefs.getIntPref("webapprt.app_update_interval");
> +      } catch (e) {
> +        this._updateInterval = 24 * 60 * 60;
> +      }

Instead of catching an exception if the pref doesn't exist, add it to the default prefs file in webapprt/prefs.js.
Attachment #8453491 - Flags: feedback?(myk) → feedback+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> Comment on attachment 8453491 [details] [diff] [review]
> Patch
> 
> Review of attachment 8453491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/webapps/tests/app.sjs
> @@ +113,5 @@
> > +  }
> > +
> > +  if ("getPackageQueries" in query) {
> > +    response.setHeader("Content-Type", "text/plain", false);
> > +    response.write(String(Number(getState("getPackageQueries"))));
> 
> Why convert it to a number only to convert it back to a string?  It should
> already be the appropriate string, given the code that sets it above.
> 
> (Also, response.write will probably stringify a number correctly anyway, so
> even if we needed to convert it to a number, we probably wouldn't need to
> convert it back to a string.)

I did this because I wanted it to return 0 the first time (otherwise it returns "").
Passing a number to response.write didn't work.

> 
> ::: toolkit/webapps/tests/test_packaged_update_service.xul
> @@ +94,5 @@
> > +  let profileDir = OS.Path.join(testAppInfo.installPath, profileRelPath);
> > +
> > +  // Force the update timer to fire in 1 second
> > +  let prefsFile = OS.Path.join(profileDir, "prefs.js");
> > +  yield writeToFile(prefsFile, "user_pref(\"webapprt.app_update_interval\", 1);");
> 
> Given this interval, which is the same amount of time we wait while
> PackageQueries or ManifestQueries don't have their expected values (or the
> app hasn't been closed yet), could another update request race the first one
> while we're running the test?

Yes, I think it may happen. We could modify the test to check >= 2 instead of == 2.

> Perhaps a better approach would be to move the updateTimer callback to a
> public method of the WebappRT object:
> 
> this.WebappRT = {
>   …
>   startUpdateService: function() {
>     …
>     timerManager.registerTimer("updateTimer",
> this.checkForUpdate.bind(this), 24 * 60 * 60);
>   },
>   
>   checkForUpdate: function() {
>     …
>   },
> };
> 
> Then call it directly from the test:
> 
>   WebappRT.checkForUpdate();
> 
> That wouldn't ensure that the timer manager correctly calls the callback;
> but that seems like it should be the responsibility of a timer manager test
> anyway.  Whereas our concern here is making sure our callback behaves as
> expected.

The problem is that we're in a different process, so we can't call WebappRT methods.

> 
> ::: webapprt/WebappRT.jsm
> @@ +94,5 @@
> > +      try {
> > +        this._updateInterval = Services.prefs.getIntPref("webapprt.app_update_interval");
> > +      } catch (e) {
> > +        this._updateInterval = 24 * 60 * 60;
> > +      }
> 
> Instead of catching an exception if the pref doesn't exist, add it to the
> default prefs file in webapprt/prefs.js.

Right ;)
Posted patch PatchSplinter Review
Added the equivalent hosted test and made sure this works on all platforms.
Attachment #8453491 - Attachment is obsolete: true
Attachment #8454210 - Flags: review?(myk)
Comment on attachment 8454210 [details] [diff] [review]
Patch

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

Please push this to tryserver before landing it!  I want to make sure it also succeeds on the slower machines in the build farm.
Attachment #8454210 - Flags: review?(myk) → review+
https://hg.mozilla.org/mozilla-central/rev/89076de4e3c3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.