991883 - [OS.File] Move test_constants from main_test_osfile_async.js to an xpcshell test

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

We want to get rid of file main_test_osfile_async.js. For this purpose, we need to move a number of tests to their own files. In this bug, we'll start with function test_constants.

The tests are currently in this file:
http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js

The objective of this bug is turn this function into an xpcshell test toolkit/components/osfile/tests/xpcshell/test_constants.js

For more details about xpcshell tests, see https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests

Comment 1

[Kushagra Singh [:kushagra]]

hi yoric :) i feel that this bug and the bug i am currently working on should go hand in hand. can i work on this ?

Comment 2

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

Can you finish the other bug first?

Comment 3

[Kushagra Singh [:kushagra]]

haha :) definitely. will come back to this when the other one is all wrapped up .

Comment 4

[Kushagra Singh [:kushagra]]

Attachment: 991883_V1.patch

Initial patch. Please go through it and let me know what changes are to be made.

Comment 5

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

Comment on attachment 8405335 [details] [diff] [review]
991883_V1.patch

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

Does this still pass tests?

For the old test:
./mach mochitest-chrome toolkit/componnets/osfile/tests/mochi/test_osfile_async.xul

For the new test:
./mach xpcshell-test toolkit/components/osfile/tests/xpcshell/test_constants.js

Also, while we are here, it would be great to take the opportunity to ensure that the values of OS.Constants.Sys are correct.

1/ To find out if we are in a DEBUG build, use:
Components.classes["@mozilla.org/xpcom/debug;1"].getService(Components.interfaces.nsIDebug2).isDebugBuild;

OS.Constants.Sys.DEBUG should be defined and |true| iff we are in a DEBUG build.

2/ To find out the name of the system, use:
Components.utils.import("resource://gre/modules/Services.jsm", this);

OS.Constants.Sys.Name should be equal to Services.appinfo.OS

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ -181,5 @@
> -    test.isnot(OS.Constants.Path, null, "OS.Constants.Path exists");
> -    test.isnot(OS.Constants.Sys, null, "OS.Constants.Sys exists");
> -  });
> -});
> -

I suspect that you forgot to remove the lines of code that calls test_constants and test_path.

::: toolkit/components/osfile/tests/xpcshell/test_constants.js
@@ +9,5 @@
> +
> +// Test that OS.Constants is defined correctly.
> +add_task(function* check_definition() {
> +  do_check_true(OS.Constants!=null);
> +  do_print("OS.Constants exists");

Each of these tests is trivial, so you probably don't need the do_print to explain what they do.

@@ +19,5 @@
> +  do_print("OS.Constants.Path exists");
> +
> +  do_check_true(OS.Constants.Sys!=null);
> +  do_print("OS.Constants.Sys exists");
> +  

Nit: Whitespace.

@@ +20,5 @@
> +
> +  do_check_true(OS.Constants.Sys!=null);
> +  do_print("OS.Constants.Sys exists");
> +  
> +  do_print("All tests passed");

You don't need this final do_print. It will be printed automatically by xpcshell.

Comment 6

[Kushagra Singh [:kushagra]]

Attachment: Added the new tests.

Sorry for the delay. Though I had already submitted it.

Comment 7

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

Comment on attachment 8428401 [details] [diff] [review]
Added the new tests.

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

Looks good, with a few minor nits.

::: toolkit/components/osfile/tests/xpcshell/test_constants.js
@@ +13,5 @@
> +  do_check_true(OS.Constants!=null);
> +  do_check_true(!!OS.Constants.Win || !!OS.Constants.libc);
> +  do_check_true(OS.Constants.Path!=null);
> +  do_check_true(OS.Constants.Sys!=null);
> +  

Nit: whitespace.

@@ +18,5 @@
> +  //check system name
> +  do_check_eq(OS.Constants.Sys.Name, Services.appinfo.OS);
> +
> +  //check if using DEBUG build
> +  if(Components.classes["@mozilla.org/xpcom/debug;1"].getService(Components.interfaces.nsIDebug2).isDebugBuild == true) {

Nit: if (

@@ +21,5 @@
> +  //check if using DEBUG build
> +  if(Components.classes["@mozilla.org/xpcom/debug;1"].getService(Components.interfaces.nsIDebug2).isDebugBuild == true) {
> +    do_check_true(OS.Constants.Sys.DEBUG);
> +  }
> +  else {

Nit: } else {

Comment 8

[Kushagra Singh [:kushagra]]

Attachment: syntax changes and try server push.

try: https://tbpl.mozilla.org/?tree=Try&rev=bf9fd15420b3&pusher=[e-mail]

push is of the penultimate patch, though it should not be an issue.

Comment 9

[Kushagra Singh [:kushagra]]

https://tbpl.mozilla.org/?tree=Try&rev=cb4d84e7b985
made a new push. Missed xpcshell tests in the last one.

Comment 10

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

Comment on attachment 8428800 [details] [diff] [review]
syntax changes and try server push.

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

Looks good, thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/df292df53eb6