1018169 - [OS.File] Write a test to check if OS.Constants is accessible from a worker.

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, defect)

Description

[Kushagra Singh [:kushagra]]

The test: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_constants.js added as a part of bug 991883 only tests OS.Constants for the main thread. We also need to make sure that they function properly for a worker.

Comment 1

[Kushagra Singh [:kushagra]]

Attachment: Performing all the tests.

Unable to use the Services.jsm module from the worker. Other than that, all seems to be implemented.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8432260 [details] [diff] [review]
Performing all the tests.

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

Looks good. I'd like a few changes before we land this, though.

::: toolkit/components/osfile/tests/xpcshell/data/worker_test.js
@@ +1,1 @@
> +onmessage = function(e) {

Can you rename this file `worker_test_constants.js`?
Also, you'll need to add the public domain header.

::: toolkit/components/osfile/tests/xpcshell/test_constants_worker.js
@@ +1,1 @@
> +"use strict";

Can you merge this with test_constants.js?

Comment 3

[Kushagra Singh [:kushagra]]

Attachment: Final patch.

try: https://tbpl.mozilla.org/?tree=Try&rev=cb8c03555293

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8432528 [details] [diff] [review]
Final patch.

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

::: toolkit/components/osfile/tests/xpcshell/data/worker_test_constants.js
@@ +1,3 @@
> +/* 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/. */

Please use the Public Domain License.

::: toolkit/components/osfile/tests/xpcshell/test_constants.js
@@ +1,1 @@
>  "use strict";

Could you take the opportunity to add the Public Domain License?

@@ +30,5 @@
> +
> +function test_worker(worker) {
> +    let deferred = Promise.defer();
> +    worker.onmessage = function (e) {
> +        // parse the stringified data

Nit: Please use a layout that is consistent with the rest of the code, i.e. two spaces idents.

@@ +33,5 @@
> +    worker.onmessage = function (e) {
> +        // parse the stringified data
> +        var data = JSON.parse(e.data);
> +        // perform the tests
> +        do_check_true(data.OS_Constants!=null);

Oh, I forgot. As of yesterday, we use `equal(x, y)` (instead of `do_check_eq(x, y)`) to check for equality and `ok(b)` to check that something is true. Can you fix this?

@@ +38,5 @@
> +        do_check_true(!!data.OS_Constants_Win || !!data.OS_Constants_libc);
> +        do_check_true(data.OS_Constants_Path!=null);
> +        do_check_true(data.OS_Constants_Sys!=null);
> +        do_check_eq(data.OS_Constants_Sys_Name, Services.appinfo.OS);
> +        if (Components.classes["@mozilla.org/xpcom/debug;1"].getService(Components.interfaces.nsIDebug2).isDebugBuild == true) {

Could you factor out that long line so as to share it between both tests?

Comment 5

[Kushagra Singh [:kushagra]]

Attachment: Public license and whitespace corrections.

Left out the xpcshell do_check_* changes.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8432559 [details] [diff] [review]
Public license and whitespace corrections.

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

Almost there.

::: toolkit/components/osfile/tests/xpcshell/test_constants.js
@@ +4,3 @@
>  "use strict";
>  
> +Components.utils.import("resource://gre/modules/Promise.jsm");

Could you add a second argument `this` to Components.utils.import?

@@ +8,2 @@
>  Components.utils.import("resource://gre/modules/Services.jsm", this);
> +let debug_check = Components.classes["@mozilla.org/xpcom/debug;1"].getService(Components.interfaces.nsIDebug2).isDebugBuild;

Can you rename `debug_check` to `isDebugBuild`?

@@ +25,3 @@
>    do_check_eq(OS.Constants.Sys.Name, Services.appinfo.OS);
> +  // check if using DEBUG build
> +  if (debug_check == true) {

Just `if (debug_check) {`

@@ +34,5 @@
> +function test_worker(worker) {
> +  let deferred = Promise.defer();
> +  worker.onmessage = function (e) {
> +    // parse the stringified data
> +    var data = JSON.parse(e.data);

I'd prefer `let` to `var`.

@@ +35,5 @@
> +  let deferred = Promise.defer();
> +  worker.onmessage = function (e) {
> +    // parse the stringified data
> +    var data = JSON.parse(e.data);
> +    // perform the tests

Let's protect this as follows:

try {
  // perform the tests
  // ...
} finally {
  deferred.resolve();
}

Comment 7

[Kushagra Singh [:kushagra]]

Attachment: incremental changes.

Will we be needing a fresh try push ?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8433306 [details] [diff] [review]
incremental changes.

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

Yes, this will be needing a Try push on xpcshell, all platforms.
Looks good to me, with a trivial typo. No need to ask for another review, though.

::: toolkit/components/osfile/tests/xpcshell/test_constants.js
@@ +48,5 @@
> +      } else {
> +        do_check_true(typeof(data.OS_Constants_Sys_DEBUG) == 'undefined');
> +      }
> +    }  finally {
> +       // reslove the promise

Nit: it's "resolve".

Comment 9

[Kushagra Singh [:kushagra]]

Attachment: Ready for shipping.

try: https://tbpl.mozilla.org/?tree=Try&rev=f4ea8b59c3c9

Comment 10

[Kushagra Singh [:kushagra]]

All green.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This is bitrotted :(. Please rebase.

Comment 12

[Kushagra Singh [:kushagra]]

Attachment: Rebased.

Comment 13

[Kushagra Singh [:kushagra]]

Apologies.

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e28e73446d88

Comment 15

[Ed Morley [:emorley]]

Backed out changeset e28e73446d88 for Android & B2G xpcshell failures in worker_test_constants.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41191283&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41189484&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41194878&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/445724f95b28

David, would you mind advising next steps for Kushagra? :-)
The original try push wasn't run on Android/B2G (https://tbpl.mozilla.org/?tree=Try&rev=f4ea8b59c3c9).

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Well, step #1 is "Run your tests on Android/B2G" :)

Step #2 is « I'll check if the DOM Worker tests even work on Android/B2G, because I just realized that they are deactivated on m-c »
try: https://tbpl.mozilla.org/?tree=Try&rev=2e792c4a6479

Step #3 is « If it fails, add the following line to ensure that your test is not executed on Android/B2G »
  skip-if = (os == "android" || appname == "b2g")

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Apparently, even the worker test of the DOM module doesn't work on Android.
So, here's plan B: instead of using a `chrome.manifest`, use the raw file:// uri for your worker file.

You can find this uri by:
- using OS.File.getCurrentDirectory() to find out the current directory;
- using OS.Path.join() to add "data" and the name of your file;
- using OS.Path.toFileURI() to conver this to a file URI.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

ping?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Kushagra, are you still working on this bug or should I unassign it?

Comment 20

[Kushagra Singh [:kushagra]]

Sorry David, I am completely swamped with work right now. Lets unassign this for the moment and maybe if I get the time, I will pick it up later.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Ok, so this bug is unassigned.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

OS.File is on its way out, closing bug.