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

RESOLVED FIXED in mozilla19

Status

()

Toolkit
OS.File
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Yoric, Assigned: andreshm)

Tracking

unspecified
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Comment 1

6 years ago
I'll take a look at this.
Assignee: nobody → andres
Thanks. I have put examples of usage in the documentation of OS.File: https://developer.mozilla.org/en/JavaScript_OS.File
(Assignee)

Comment 3

6 years ago
Created attachment 674073 [details] [diff] [review]
Patch v1
Attachment #674073 - Flags: review?(dteller)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
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.
Attachment #674073 - Flags: review?(dteller) → feedback+
Cc-ing Paolo, who might be interested to see what we are doing with his baby :)
(Assignee)

Comment 6

6 years ago
Created attachment 674286 [details] [diff] [review]
Patch v2
Attachment #674073 - Attachment is obsolete: true
Attachment #674286 - Flags: review?(dteller)
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 :)
Attachment #674286 - Flags: review?(dteller) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2953f14597
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d2953f14597
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.