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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: 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.

Comment 2

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

(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 3

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

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.

Comment 4

[Sankha Narayan Guria [:sankha]]

Attachment: 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.

Comment 5

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

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.

Comment 6

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

Any news?

Comment 7

[Sankha Narayan Guria [:sankha]]

Attachment: Patch with suggested changes

Once this test is added to the tree I will start working with bug 750177.

Comment 8

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

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.

Comment 9

[Sankha Narayan Guria [:sankha]]

Attachment: Patch

Comment 10

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

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)

Comment 11

[Sankha Narayan Guria [:sankha]]

Attachment: Final patch

TryServer: https://tbpl.mozilla.org/?tree=Try&rev=2472d82ec089

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68f8f4b74f63

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68f8f4b74f63