800697 - [OS.File] Convert async testsuite to use Task.js

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

bug 763311 introduces Task.js-like API on top of promises. We should make use of it to simplify the async testsuite of OS.File.

Comment 1

[Andres Hernandez [:andreshm]]

I'll take a look at this.

Comment 2

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

Thanks. I have put examples of usage in the documentation of OS.File: https://developer.mozilla.org/en/JavaScript_OS.File

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 4

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

Comment on attachment 674073 [details] [diff] [review]
Patch v1

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

I would like a few changes, but this looks awesome. Great work, Andres!

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +145,5 @@
> +      return aux();
> +    }
> +  };
> +  return aux();
> +});

Now that:
- we are using Task.js;
- maketest ensures that every test returns a resolved promise;

I have the feeling that you can turn this recursive loop in a (possibly outlandish) for loop.

Something along the lines of:

«
for (let i of tests) {
   yield test(); // Test is always a resolved promise
}
SimpleTest.finish();
»

Could you see if this works?

@@ +211,5 @@
>  
>      // Attempt to open a file correctly
> +    test.info("Attempting to open a file correctly");
> +    let openedFile = yield OS.File.open(EXISTING_FILE);
> +    test.ok(true, "File opened correctly");

Could you add newlines before these |test.info| that announce new tests?

@@ +230,5 @@
> +    test.info("Stating file");
> +    let stat1 = yield file.stat();
> +    test.ok(true, "stat has worked " + stat1);
> +    test.ok(stat1, "stat is not empty");
> +    yield file.close();

As a rule of thumb, occurrences of |always| should be become |try ... finally|.

@@ +258,4 @@
>  
> +    let fileSource = yield OS.File.open(pathSource);
> +    test.info("Input file opened");
> +    test.info("OS.Constants.Path is " + OS.Constants.Path.toSource());

This line logging |OS.Constants.Path.toSource()| looks deprecated. Could you take the opportunity to remove it?

@@ +277,4 @@
>  
>      // Test read
> +    yield fileSource.setPosition(0);
> +    let result = yield fileSource.read();

Now that |result| has a wider scope, you should find a more meaningful name. Maybe |readAllResult|?

@@ +286,2 @@
>  
>      // Close stuff

Again, these files should be closed in a |try ... finally|.

@@ +304,4 @@
>      let tmpPath = pathDest + ".tmp";
>  
> +    // Check that read + writeAtomic performs a correct copy
> +    let path = yield OS.File.getCurrentDirectory();

Now that the scope has grown, I would rename |path|, for instance to |currentDir| or |currentPath|.

@@ +304,5 @@
>      let tmpPath = pathDest + ".tmp";
>  
> +    // Check that read + writeAtomic performs a correct copy
> +    let path = yield OS.File.getCurrentDirectory();
> +    test.ok(path, "Obtained current directory");

Let's get rid of this |test.ok(path, ...)|, it is not really useful anymore.

@@ +315,2 @@
>  
> +    // Check that options are not altered

Move this comment after the test on the number of bytes.

@@ +338,5 @@
> +      test.ok(err instanceof OS.File.Error, "writeAtomic correctly failed with a file error");
> +      test.ok(err.becauseExists, "writeAtomic file error confirmed that the file already exists");
> +    }
> +    yield reference_compare_files(pathSource, pathDest, test);
> +    test.info("With noOverwrite, writeAtomic correctly did not overwrite destination file");

I think that this |test.info| is incorrect.

@@ +364,5 @@
> +      yield OS.File.writeAtomic(pathDest, contents, {});
> +      test.fail("Without a tmpPath, writeAtomic should have failed");
> +    } catch (err) {
> +      test.ok(true, "Without a tmpPath, writeAtomic has failed as expected");
> +    }

That is so much nicer than the original version!

@@ +401,2 @@
>  
> +    yield file.close();

As above, |always| => |finally|.

@@ +407,5 @@
> + * Test OS.File.prototype.{copy, move}
> + */
> +let test_copy = maketest("copy", function copy(test) {
> +  return Task.spawn(function() {
> +    let path = yield OS.File.getCurrentDirectory();

Let's rename this one, too.

@@ +408,5 @@
> + */
> +let test_copy = maketest("copy", function copy(test) {
> +  return Task.spawn(function() {
> +    let path = yield OS.File.getCurrentDirectory();
> +    test.ok(path, "Obtained current directory");

Let's get rid of this one, too.

@@ +458,5 @@
> +    } catch (err) {
> +      test.ok(err, "Removing an absent directory without ignoreAbsent throws the right error");
> +      test.ok(err instanceof OS.File.Error, "Error is an OS.File.Error");
> +      test.ok(err.becauseNoSuchFile, "Error mentions that the file does not exist");
> +    }

Nice change.

Comment 5

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

Cc-ing Paolo, who might be interested to see what we are doing with his baby :)

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Comment 7

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

Comment on attachment 674286 [details] [diff] [review]
Patch v2

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

Thank you for another great patch!

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +140,5 @@
> +    yield test_exists();
> +    info("Test is over");
> +    SimpleTest.finish();
> +  });
> +});

Ok, that works, too :)

Comment 8

[Andres Hernandez [:andreshm]]

Thanks!

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=efa2610b32da

Comment 9

[Andres Hernandez [:andreshm]]

All green! :) https://tbpl.mozilla.org/?tree=Try&rev=efa2610b32da

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2953f14597

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d2953f14597