[OS.File] Add sanity tests for OS.Constants.{libc, Win}

RESOLVED FIXED in mozilla21

Status

()

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

People

(Reporter: Yoric, Assigned: sankha)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

OS.Constants.Path has tests, but OS.Constants.libc and OS.Constants.Win don't really have tests. We should add them.
(Assignee)

Comment 1

5 years ago
Created attachment 694789 [details] [diff] [review]
Tests for OS.Constants.{libc, Win}

I tried to add the tests initially to the Chrome Worker, but it did not work. Ms2ger said that |Components| can only be used from the main thread, so I moved the tests to the test_constants.xul file.
Attachment #694789 - Flags: review?(dteller)
(In reply to Sankha Narayan Guria from comment #1)
> Created attachment 694789 [details] [diff] [review]
> Tests for OS.Constants.{libc, Win}
> 
> I tried to add the tests initially to the Chrome Worker, but it did not
> work. Ms2ger said that |Components| can only be used from the main thread,
> so I moved the tests to the test_constants.xul file.

This should be sufficient.
Comment on attachment 694789 [details] [diff] [review]
Tests for OS.Constants.{libc, Win}

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

Looks good, want more :)

::: dom/system/tests/test_constants.xul
@@ +33,5 @@
>  }
>  
> +// Test that OS.Constants.libc is defined
> +function test_libc() {
> +  isnot(null, OS.Constants.libc, "OS.Constants.libc is defined");

Could you also check some well-known values? e.g.
OS.Constants.libc.S_IXOTH == 0001

@@ +41,5 @@
> +function test_Win() {
> +  var xulRuntime = Components.classes["@mozilla.org/xre/app-info;1"]
> +                           .getService(Components.interfaces.nsIXULRuntime);
> +  if(xulRuntime.OS == "Windows")
> +    isnot(null, OS.Constants.Win, "OS.Constants.Win is defined");

I would rather use |is("Win" in OS.Constants, xulRuntime.OS == "Windows", ...)|.

Could you also check some well-known values? e.g.
OS.Constants.Win.INVALID_HANDLE_VALUE == -1

@@ +52,5 @@
>    Components.classes["@mozilla.org/net/osfileconstantsservice;1"].
>      getService(Components.interfaces.nsIOSFileConstantsService).
>      init();
>    Components.utils.import("resource://gre/modules/ctypes.jsm");
> +  

Nit: useless whitespace.
Attachment #694789 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 694885 [details] [diff] [review]
Patch with tests for well-known values

Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=8554396f8d86
to check whether tests pass successfully on Windows.
Attachment #694789 - Attachment is obsolete: true
Attachment #694885 - Flags: review?(dteller)
Comment on attachment 694885 [details] [diff] [review]
Patch with tests for well-known values

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

::: dom/system/tests/test_constants.xul
@@ +51,5 @@
> +// Test that OS.Constants.Win is defined
> +function test_Win() {
> +  var xulRuntime = Components.classes["@mozilla.org/xre/app-info;1"]
> +                           .getService(Components.interfaces.nsIXULRuntime);
> +  is("Win" in OS.Constants, xulRuntime.OS == "Windows", "OS.Constants.Win is defined");

After this test, it is probably simpler to bail out if xulRuntime.OS is not "Windows".

@@ +52,5 @@
> +function test_Win() {
> +  var xulRuntime = Components.classes["@mozilla.org/xre/app-info;1"]
> +                           .getService(Components.interfaces.nsIXULRuntime);
> +  is("Win" in OS.Constants, xulRuntime.OS == "Windows", "OS.Constants.Win is defined");
> +  is("Win.INVALID_HANDLE_VALUE" in OS.Constants, xulRuntime.OS == "Windows" && -1,

&& -1 ? That looks fishy.
Attachment #694885 - Flags: review?(dteller) → feedback+
Any news?
Flags: needinfo?(sankha93)
(Assignee)

Comment 7

5 years ago
Created attachment 700190 [details] [diff] [review]
Patch with suggested changes

Once this test is added to the tree I will start working with bug 750177.
Attachment #694885 - Attachment is obsolete: true
Attachment #700190 - Flags: review?(dteller)
Flags: needinfo?(sankha93)
Comment on attachment 700190 [details] [diff] [review]
Patch with suggested changes

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

::: dom/system/tests/test_constants.xul
@@ +54,5 @@
> +                           .getService(Components.interfaces.nsIXULRuntime);
> +  if(xulRuntime.OS == "Windows") {
> +    ok("Win" in OS.Constants, "OS.Constants.Win is defined");
> +    is("Win.INVALID_HANDLE_VALUE" in OS.Constants, -1,
> +       "OS.Constants.Win.INVALID_HANDLE_VALUE is defined");

That test won't work. Did you mean
  is (OS.Constants.Win.INVALID_HANDLE_VALUE, -1, "...")
?

Also, we don't just check whether the value is defined, we check whether it is correct.
Attachment #700190 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 9

5 years ago
Created attachment 702375 [details] [diff] [review]
Patch
Attachment #700190 - Attachment is obsolete: true
Attachment #702375 - Flags: review?(dteller)
Comment on attachment 702375 [details] [diff] [review]
Patch

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

That looks good. You have my r+ provided this pass tests.
(don't forget to change the title of the patch, too, before we can land it)
Attachment #702375 - Flags: review?(dteller) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/68f8f4b74f63
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.