885480 - [OS.File] OS.File.open results in platform-dependent error attributes

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

Steps to reproduce:

- Attempt to open a file using OS.File.open, providing a path to a directory that does not exist. 

- Check whether the error produced has attribute becauseNoSuchFile.

- Perform this check both on a unix based file system and a windows system.

Expected behavior: error has attribute set on all platforms.
Actual: becauseNoSuchFile is set on unix based systems, but not on windows.

Comment 1

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

I don't have time to work on this at the moment, but if you want to help with this, Chris, I'll be happy to mentor/review.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Sure, I'll be happy to take a look - could be a straightforward fix. My semester's gone a bit hectic, but I should be able to look at this in the coming week.

Comment 3

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

If you need any help, feel free to needinfo? me.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Set becauseNoSuchFile on windows for osfile errors produced as a result of no such directory;

I don't have easy access to a windows machine, but here's a try run with the
test without the fix to confirm its failure:

https://tbpl.mozilla.org/?tree=Try&rev=1204928e3511

And one with the fix applied to confirm its success:

https://tbpl.mozilla.org/?tree=Try&rev=6abf2f658cbf

I'll flag for review based on their results.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

It looks as though this has had the desired effect.

Comment 6

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

Comment on attachment 823117 [details] [diff] [review]
Set becauseNoSuchFile on windows for osfile errors produced as a result of no such directory;

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

r=me for the code
However, please don't put the test in test_exception.js, that (admittedly undocumented) file is for something else entirely. You could create a new test_open.js for this purpose.
Bonus points if you take the opportunity to migrate test_open() from mochi/main_test_osfile_async.js to this new file.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: Set becauseNoSuchFile on windows for osfile errors produced as a result of no such directory;

This makes the requested changes to test files. I don't know if you have a
preference to keep vacuous "ok(true)" assertions vs. just printing a message.
I've favored simply printing a message here.

Comment 8

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

Comment on attachment 823436 [details] [diff] [review]
Set becauseNoSuchFile on windows for osfile errors produced as a result of no such directory;

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

r=me, with the minor changes below

::: toolkit/components/osfile/tests/xpcshell/test_open.js
@@ +20,5 @@
> +  // Attempt to open a file that does not exist, ensure that it yields the
> +  // appropriate error.
> +  try {
> +    let fd = yield OS.File.open(OS.Path.join(".", "This file does not exist"));
> +    do_check_true(false, "File opening 1 succeeded (it should fail)" + fd);

That's not going to work exactly as you intend, as do_check_true throws an exception if it fails.

@@ +21,5 @@
> +  // appropriate error.
> +  try {
> +    let fd = yield OS.File.open(OS.Path.join(".", "This file does not exist"));
> +    do_check_true(false, "File opening 1 succeeded (it should fail)" + fd);
> +  } catch (err) {

Let's take the opportunity to make this a
 catch (err if err instanceof OS.File.Error && err.becauseNoSuchFile) {

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: Set becauseNoSuchFile on windows for osfile errors produced as a result of no such directory;

I think this works as intended for the case mentioned above: if trying to open
the file doesn't throw an exception, do_check_true(false) fails the test.
If it does, and it doesn't fulfill the instanceof and attribute check, then
the exception will go uncaught and fail the test.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Attachment: Set becauseNoSuchFile on windows for osfile errors produced as a result of no such directory;

Fix commit message.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/49a1c783b223

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49a1c783b223

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49a1c783b223