[OS.File] Share code between tests

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Yoric, Assigned: sankha)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

At the moment:
- functions ok, is, isnot, ... are defined several times in the testsuite of OS.File;
- communication between worker thread and main thread are defined several times in this testsuite.

This is error-prone. We should remove these duplicates.
Assignee: nobody → sankha93
Posted patch Test functions in one file (obsolete) — Splinter Review
I have moved all the common test functions to a common file. Please take a look.
Attachment #687707 - Flags: feedback?(dteller)
Comment on attachment 687707 [details] [diff] [review]
Test functions in one file

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

This looks good, for the worker-side.
Now, there is also some code of test_osfile_front.xul, test_osfile_back.xul and test_osfile_comms.xul that could be shared.

::: toolkit/components/osfile/tests/mochi/Makefile.in
@@ +13,5 @@
>  MODULE          = osfile
>  
>  MOCHITEST_CHROME_FILES := \
>    test_osfile_back.xul \
> +  worker_test_osfile_functions.js \

I would call it "worker_test_osfile_shared.js".

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_functions.js
@@ +6,5 @@
> +}
> +
> +function ok(condition, description) {
> +  send({kind: "ok", condition: condition, description:description});
> +}

Could you take the opportunity to normalize |condition| to a boolean and |description| to a string? This will avoid problems when communicating between threads.
Note: to normalize to a boolean, use a double negation, and to normalize to a string, prepend "".

Also, same thing for |is|, |isnot|, |info|.
Attachment #687707 - Flags: feedback?(dteller) → feedback+
Posted patch Test Suite common code shared (obsolete) — Splinter Review
I have made the necessary changes and also shifted the |worker.onmessage| common in the tests to worker_handler.js
Attachment #687707 - Attachment is obsolete: true
Attachment #688101 - Flags: review?(dteller)
Comment on attachment 688101 [details] [diff] [review]
Test Suite common code shared

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

Looks good. Just a few changes and we'll be ready.

::: toolkit/components/osfile/tests/mochi/worker_handler.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

This looks good. However, worker_handler_comms is used only once, so I would not bother with moving that function away from test_osfile_comms.xul

@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function worker_handler(worker) {
> +  worker.onmessage = function(msg) {
> +    ok(true, "MAIN: onmessage "+JSON.stringify(msg.data));

Nit: I should have added a whitespace before and after +, could you do this for me?

@@ +25,5 @@
> +    default:
> +      SimpleTest.ok(false, "test_osfile.xul: wrong message "+JSON.stringify(msg.data));
> +      return;
> +    }
> +  };

You could even go a little further and set worker.onerror as part of worker_handler

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_shared.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function log(text) {
> +  dump("WORKER "+text+"\n");
> +}

log might not be used anymore. Could you double-check and possibly remove it completely if is not used anymore?
Attachment #688101 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> log might not be used anymore. Could you double-check and possibly remove it
> completely if is not used anymore?

It is used in the test suite here and there and largely in the worker_test_osfile_win.js. So should I keep it?
Posted patch Fixed patchSplinter Review
Attachment #688101 - Attachment is obsolete: true
Attachment #690070 - Flags: review?(dteller)
Comment on attachment 690070 [details] [diff] [review]
Fixed patch

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

You already have my r+, no need to ask again.
Attachment #690070 - Flags: review?(dteller) → review+
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][checkin-needed]
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][checkin-needed] → [mentor=Yoric][lang=js]
https://hg.mozilla.org/mozilla-central/rev/dcef2011bf4f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.