[OS.File] Be smarter with functions that can fail with becauseAbsent/becauseExists.

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Yoric, Assigned: hardfire)

Tracking

({dev-doc-needed})

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

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

By default, OS.File.createDir should succeed if the directory already exists.
By default, OS.File.removeEmptyDir should succeed if the directory doesn't exist yet.

This will result in cleaner code and less user errors.
Assignee: nobody → karim
Hi Karim, how is that bug progressing? Do you need any assistance?
Flags: needinfo?(karim)
In the absence of news, de-assigning.
Assignee: karim → nobody
Flags: needinfo?(karim)
Assignee: nobody → avinash
Posted patch patch for 897023 (obsolete) — Splinter Review
I tried to modify the test accordingly too, didn't know how to test it though.
Attachment #819598 - Flags: review?(dteller)
Comment on attachment 819598 [details] [diff] [review]
patch for 897023

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

The code looks good.
Regarding the test, could you please rather make it an xpcshell test (see directory toolkit/component/osfile/tests/xpcshell)? We already test way too many things in main_test_osfile_async.js, we want to progressively get rid of it.
To execute these tests, run
 ./mach xpcshell-test path/to/your/test.js

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +364,5 @@
>         if (!result) {
>           if (options.ignoreAbsent &&
>               ctypes.winLastError == Const.ERROR_FILE_NOT_FOUND) {
>             return;
>           }

Did you forget to patch that method?
Attachment #819598 - Flags: review?(dteller) → feedback+
Any news about that bug?
Flags: needinfo?(avinash)
Also, I believe that we should update function OS.File.remove() so that it doesn't fail when the file doesn't exist.
yes, I have updated the function, have to werite the xpc tests still, was out for the festival so couldn't work on this, will hopefully send a patch by tomorrow
Flags: needinfo?(avinash)
My first time writing a xpcshell test. Hope i have followed the right conventions.
Attachment #819598 - Attachment is obsolete: true
Attachment #825168 - Flags: review?(dteller)
Comment on attachment 825168 [details] [diff] [review]
updated function and added xpcshell-test

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

Looks good to me, with a few minor changes.
Could you take the opportunity to remove test_mkdir from main_test_osfile_async.js?

::: toolkit/components/osfile/tests/xpcshell/test_osfile_makeDir.js
@@ +1,1 @@
> +"use strict";

Nit: just "test_makeDir.js" would be better.
Also, please add the public domain copyright notice: http://www.mozilla.org/MPL/headers/

Also, could you add two lines of documentation mentioning that you're testing OS.File.makeDir?

@@ +1,4 @@
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/osfile.jsm");
> +Components.utils.import("resource://gre/modules/Task.jsm");

You don't need Task.jsm.

@@ +29,5 @@
> +
> +  //check if the directory exists
> +  yield OS.File.stat(dir);
> +
> +  // Make a directory that exists

Nit: Make a directory that *already* exists

@@ +44,5 @@
> +    exception = ex;
> +  }
> +
> +  do_check_true(!!exception);
> +  do_check_true(exception instanceof OS.File.Error);

Could you also check that |exception.becauseExists| is true?

::: toolkit/components/osfile/tests/xpcshell/test_osfile_removeEmptyDir.js
@@ +1,1 @@
> +"use strict";

Same remarks as in the other file.

@@ +38,5 @@
> +    exception = ex;
> +  }
> +
> +  do_check_true(!!exception);
> +  do_check_true(exception instanceof OS.File.Error);

Could you test that |exception.becauseNoSuchFile| is true?
Attachment #825168 - Flags: review?(dteller) → review+
Attachment #825168 - Attachment is obsolete: true
Attachment #827911 - Flags: review?(dteller)
Comment on attachment 827911 [details] [diff] [review]
updated patch - changed filenames, comments and added test conditions

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

Looks good to me. Have you run that through the Try Server yet?
Note for later: could you number the successive versions of your patches?
Attachment #827911 - Flags: review?(dteller) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3232989e79e7
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][Async:P2] → [mentor=Yoric][lang=js][Async:P2][fixed-in-fx-team]
Backed out for mochitest-other failures. Please ensure that you run the appropriate tests in your Try pushes.
https://hg.mozilla.org/integration/fx-team/rev/d4685e3d278e

https://tbpl.mozilla.org/php/getParsedLog.php?id=30419099&tree=Fx-Team
Flags: in-testsuite+
Whiteboard: [mentor=Yoric][lang=js][Async:P2][fixed-in-fx-team] → [mentor=Yoric][lang=js][Async:P2]
Posted patch 897023-v4 (obsolete) — Splinter Review
removed the mochi test which i missed removing earlier and that was causing the mochi-test to fail.
Attachment #827911 - Attachment is obsolete: true
Attachment #831779 - Flags: review?(dteller)
Comment on attachment 831779 [details] [diff] [review]
897023-v4

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

Looks good.
Please don't forget to
1/ change your commit message to
 Bug 897023 - [OS.File] Be smarter with functions that can fail with becauseAbsent/becauseExists;r=yoric
2/ run the tests.
Attachment #831779 - Flags: review?(dteller) → review+
Changed commit message. Tests pass locally. I don't have access to the try servers yet so cannot test there.
Attachment #831779 - Attachment is obsolete: true
Attachment #832507 - Flags: review?(dteller)
Comment on attachment 832507 [details] [diff] [review]
897023-v4-updated_commit_message

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=a10feb11877d
Attachment #832507 - Flags: review?(dteller) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9429e63ae823
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][Async:P2] → [mentor=Yoric][lang=js][Async:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9429e63ae823
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][Async:P2][fixed-in-fx-team] → [mentor=Yoric][lang=js][Async:P2]
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.