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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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.

Comment 1

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

Hi Karim, how is that bug progressing? Do you need any assistance?

Comment 2

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

In the absence of news, de-assigning.

Comment 3

[Avinash [:hardfire]]

Attachment: patch for 897023

I tried to modify the test accordingly too, didn't know how to test it though.

Comment 4

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

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?

Comment 5

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

Any news about that bug?

Comment 6

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

Also, I believe that we should update function OS.File.remove() so that it doesn't fail when the file doesn't exist.

Comment 7

[Avinash [:hardfire]]

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

Comment 8

[Avinash [:hardfire]]

Attachment: updated function and added xpcshell-test

My first time writing a xpcshell test. Hope i have followed the right conventions.

Comment 9

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

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?

Comment 10

[Avinash [:hardfire]]

Attachment: updated patch - changed filenames, comments and added test conditions

Comment 11

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

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?

Comment 12

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=59ce02f93c1b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3232989e79e7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Avinash [:hardfire]]

Attachment: 897023-v4

removed the mochi test which i missed removing earlier and that was causing the mochi-test to fail.

Comment 16

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

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.

Comment 17

[Avinash [:hardfire]]

Attachment: 897023-v4-updated_commit_message

Changed commit message. Tests pass locally. I don't have access to the try servers yet so cannot test there.

Comment 18

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

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9429e63ae823

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9429e63ae823