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

NEW
Unassigned

Status

()

Toolkit
OS.File
4 years ago
3 years ago

People

(Reporter: kushagra, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good second bug])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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.
Summary: [OS.File] Check if OS.Constants is accessible from a worker. → [OS.File] Write a test to check if OS.Constants is accessible from a worker.
Whiteboard: [mentor=Yoric][lang=js][good second bug]
(Reporter)

Updated

4 years ago
Assignee: nobody → singh.kushagra93
Status: NEW → ASSIGNED
(Reporter)

Comment 1

4 years ago
Created attachment 8432260 [details] [diff] [review]
Performing all the tests.

Unable to use the Services.jsm module from the worker. Other than that, all seems to be implemented.
Attachment #8432260 - Flags: review?(dteller)
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?
Attachment #8432260 - Flags: review?(dteller) → feedback+
(Reporter)

Comment 3

4 years ago
Created attachment 8432528 [details] [diff] [review]
Final patch.

try: https://tbpl.mozilla.org/?tree=Try&rev=cb8c03555293
Attachment #8432260 - Attachment is obsolete: true
Attachment #8432528 - Flags: review?(dteller)
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?
Attachment #8432528 - Flags: review?(dteller) → feedback+
(Reporter)

Comment 5

4 years ago
Created attachment 8432559 [details] [diff] [review]
Public license and whitespace corrections.

Left out the xpcshell do_check_* changes.
Attachment #8432528 - Attachment is obsolete: true
Attachment #8432559 - Flags: review?(dteller)
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();
}
Attachment #8432559 - Flags: review?(dteller) → feedback+
(Reporter)

Comment 7

4 years ago
Created attachment 8433306 [details] [diff] [review]
incremental changes.

Will we be needing a fresh try push ?
Attachment #8432559 - Attachment is obsolete: true
Attachment #8433306 - Flags: review?(dteller)
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".
Attachment #8433306 - Flags: review?(dteller) → review+
(Reporter)

Comment 9

4 years ago
Created attachment 8433315 [details] [diff] [review]
Ready for shipping.

try: https://tbpl.mozilla.org/?tree=Try&rev=f4ea8b59c3c9
Attachment #8433306 - Attachment is obsolete: true
(Reporter)

Comment 10

4 years ago
All green.
Keywords: checkin-needed
This is bitrotted :(. Please rebase.
Keywords: checkin-needed
(Reporter)

Comment 12

4 years ago
Created attachment 8435201 [details] [diff] [review]
Rebased.
Attachment #8433315 - Attachment is obsolete: true
(Reporter)

Comment 13

4 years ago
Apologies.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28e73446d88
Keywords: checkin-needed
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).
Flags: needinfo?(dteller)
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")
Flags: needinfo?(dteller)
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.
Flags: needinfo?(singh.kushagra93)
(Assignee)

Updated

4 years ago
Mentor: dteller@mozilla.com
Whiteboard: [mentor=Yoric][lang=js][good second bug] → [lang=js][good second bug]
ping?
Kushagra, are you still working on this bug or should I unassign it?
(Reporter)

Comment 20

3 years ago
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.
Assignee: singh.kushagra93 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(singh.kushagra93) → needinfo?(dteller)
Ok, so this bug is unassigned.
Flags: needinfo?(dteller)
You need to log in before you can comment on or make changes to this bug.