Closed Bug 885480 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: OS.File, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: chmanchester, Assigned: chmanchester)

Details

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

Attachments

(1 file, 3 obsolete files)

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.
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.
Flags: needinfo?(chmanchester)
Whiteboard: [Async][mentor=Yoric][lang=js]
Summary: OS.File.open results in platform-dependent error attributes → [OS.File] OS.File.open results in platform-dependent error attributes
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.
Flags: needinfo?(chmanchester)
If you need any help, feel free to needinfo? me.
Assignee: nobody → chmanchester
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.
It looks as though this has had the desired effect.
Attachment #823117 - Flags: review?(dteller)
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.
Attachment #823117 - Flags: review?(dteller) → feedback+
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.
Attachment #823436 - Flags: review?(dteller)
Attachment #823117 - Attachment is obsolete: true
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) {
Attachment #823436 - Flags: review?(dteller) → review+
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.
Attachment #823436 - Attachment is obsolete: true
Attachment #824044 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/49a1c783b223
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/49a1c783b223
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [Async][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.