809305 - [OS.File] Share code between tests

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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.

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: Test functions in one file

I have moved all the common test functions to a common file. Please take a look.

Comment 2

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

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|.

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: Test Suite common code shared

I have made the necessary changes and also shifted the |worker.onmessage| common in the tests to worker_handler.js

Comment 4

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

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?

Comment 5

[Sankha Narayan Guria [:sankha]]

(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?

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: Fixed patch

Comment 7

[Sankha Narayan Guria [:sankha]]

TryServer: https://tbpl.mozilla.org/?tree=Try&rev=a581128a13f0

Comment 8

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

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dcef2011bf4f