Closed Bug 991883 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: Yoric, Assigned: kushagra)

Details

(Whiteboard: [mentor=Yoric][lang=js][good first bug])

Attachments

(1 file, 2 obsolete files)

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
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 ?
Flags: needinfo?(dteller)
Can you finish the other bug first?
Flags: needinfo?(dteller)
haha :) definitely. will come back to this when the other one is all wrapped up .
Assignee: nobody → singh.kushagra93
Attached patch 991883_V1.patch (obsolete) — Splinter Review
Initial patch. Please go through it and let me know what changes are to be made.
Attachment #8405335 - Flags: review?(dteller)
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.
Attachment #8405335 - Flags: review?(dteller) → feedback+
Status: NEW → ASSIGNED
Attached patch Added the new tests. (obsolete) — Splinter Review
Sorry for the delay. Though I had already submitted it.
Attachment #8405335 - Attachment is obsolete: true
Attachment #8428401 - Flags: review?(dteller)
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 {
Attachment #8428401 - Flags: review?(dteller) → review+
try: https://tbpl.mozilla.org/?tree=Try&rev=bf9fd15420b3&pusher=singh.kushagra93@gmail.com

push is of the penultimate patch, though it should not be an issue.
Attachment #8428401 - Attachment is obsolete: true
Attachment #8428800 - Flags: review?(dteller)
https://tbpl.mozilla.org/?tree=Try&rev=cb4d84e7b985
made a new push. Missed xpcshell tests in the last one.
Comment on attachment 8428800 [details] [diff] [review]
syntax changes and try server push.

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

Looks good, thanks.
Attachment #8428800 - Flags: review?(dteller) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/df292df53eb6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][good first bug] → [mentor=Yoric][lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/df292df53eb6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][good first bug][fixed-in-fx-team] → [mentor=Yoric][lang=js][good first bug]
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.