468196 - xpcshell-tests: Port SimpleTest check API

Status: RESOLVED WONTFIX

(Testing :: XPCShell Harness, enhancement)

Description

[Jeff Walden [:Waldo]]

The current xpcshell test API is a bit weird and is fail-fast; SimpleTest is nicer and more familiar to people through Mochitest work.  *I'd* certainly rather use SimpleTest than do_* methods, at the very least!

Comment 1

[Serge Gautherie (:sgautherie)]

I already had this idea lurking,
but I just noticed at least 2 tests which abuse the do_*() api by providing a text/string for the "internal" 'stack' parameter :-/

Bug 362433 could work _a little_ around that,
but this bug would be a better solution.

***

Current:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js
Wanted:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js

What is the goal here: to share a common SimpleTest.js or to duplicate its "api" only?

Comment 2

[Jeff Walden [:Waldo]]

Duplicate the API, the reporting methods would be completely different.  It's certainly possible to abstract out the reporting, but I don't think it's worth the effort to share so little code, personally.

Comment 3

[Serge Gautherie (:sgautherie)]

The basic API would be: ok(), is(), isnot(), todo(), todo_is(), todo_isnot().
Anything else?

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Add and switch (harness) to new API

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090609 Minefield/3.6a1pre] (mozilla-central-win32-unittest/1244597186) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/5d1bf7e2c8dd)

Works fine.

*****

(In reply to comment #3)
> Anything else?

Shall I alias do_test_pending() + do_test_finished() to waitForExplicitFinish() + finish(),
though xpcshell-tests can call them multiple times whereas mochitest can't?

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Add and switch (harness) to new API

Av1, revised and unbitrotted.

Comment 6

[Serge Gautherie (:sgautherie)]

jwalden, ping for review ?

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 383127 [details] [diff] [review]
(Av1a) Add and switch (harness) to new API

>diff --git a/testing/xpcshell/example/unit/test_load_httpd_js.js b/testing/xpcshell/example

> function run_test() {
>   var httpserver = new nsHttpServer();
>-  do_check_neq(httpserver, null);
>-  do_check_neq(httpserver.QueryInterface(Components.interfaces.nsIHttpServer), null);
>+  ok(httpserver, "httpserver");
>+  ok(httpserver.QueryInterface(Components.interfaces.nsIHttpServer),
>+       "httpserver.QueryInterface()", httpserver);

Why the third argument here?


>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js

>@@ -132,23 +133,59 @@ function _execute_test() {
>   }
> 
>   // _TAIL_FILES is dynamically defined by <runxpcshelltests.py>.
>   _load_files(_TAIL_FILES);
> 
>   if (!_passed)
>     return;
> 
>+  var trueTodoChecks = _todoChecks;
>+
>+  // ToDo: remove test and related API when all tests have been updated. (Bug Nnn)
>+  if (_usedDeprecatedAPI) {
>+    dump("TEST-KNOWN-FAIL | (xpcshell/head.js) | " +
>+           "This test uses some deprecated do_*() API.\n");

"This test uses deprecated do_*() checks rather than is/isnot/ok/etc. tests.\n"


>+    ++_todoChecks;
>+  }
>+
>   var truePassedChecks = _passedChecks - _falsePassedChecks;
>-  if (truePassedChecks > 0)
>+  if (truePassedChecks == 0 && trueTodoChecks == 0)
>+    // ToDo: switch to TEST-UNEXPECTED-FAIL when all tests have been updated. (Bug 496443)
>+    dump("TEST-INFO | (xpcshell/head.js) | No (+ " + _falsePassedChecks +
>+           ") checks actually run\n");
>+  else {

Please put braces around the if-arm above since the else-arm has braces around it.


>+function _logResult(test, aPassString, aFailString, aStack)
>+{
>+  dump((test.result ? aPassString : aFailString) + " | " +
>+         aStack.filename + " | " + test.text +
>+         (!test.result && test.diag ? " - " + test.diag : "") + "\n");
>+
>+  if (test.result)
>+    if (test.todo)
>+      // 'todo' passed unexpectedly.
>+      _passed = false;
>+    else
>+      // true check passed as expected.
>+      ++_passedChecks;
>   else

Oh gag.  Never, under any circumstances, should you omit braces in an if-else around a non-trivial statement like this.  I don't remember how the parsing works, but that first else could easily correspond to the outer |if| if you didn't have indentation as a guide (red herring?), and every reader is going to do a double-take on this unless you put braces around |if (test.todo) ... else ...|.  It probably does work the way your code expects it should work, but no reason at all to take a chance.


>-    // ToDo: switch to TEST-UNEXPECTED-FAIL when all tests have been updated. (Bug 496443)
>-    dump("TEST-INFO | (xpcshell/head.js) | No (+ " + _falsePassedChecks + ") checks actually run\n");
>+    if (test.todo)
>+      // 'todo' failed as expected.
>+      ++_todoChecks;
>+    else
>+      // true check failed unexpectedly.
>+      _passed = false;

And braces again around the body of the else as well.


>     if (!allowNonexistent && !lf.exists()) {
>-      // Not using do_throw(): caller will continue.
>-      _passed = false;
>       var stack = Components.stack.caller;
>-      dump("TEST-UNEXPECTED-FAIL | " + stack.filename + " | [" +
>-             stack.name + " : " + stack.lineNumber + "] " + lf.path +
>-             " does not exist\n");
>+      ok(false,
>+         "[" + stack.name + " : " + stack.lineNumber + "] " + lf.path +
>+           " does not exist.",
>+         null, stack);
>+      // Not throwing: caller will continue.

This comment is unnecessary and obvious from the method argument name.


>@@ -286,43 +327,49 @@ function do_get_cwd() {
>  * Loads _HTTPD_JS_PATH file, which is dynamically defined by
>  * <runxpcshelltests.py>.
>  */
> function do_load_httpd_js() {
>   load(_HTTPD_JS_PATH);
> }
> 
> function do_load_module(path) {
>-  var lf = do_get_file(path);
>-  const nsIComponentRegistrar = Components.interfaces.nsIComponentRegistrar;
>-  do_check_true(Components.manager instanceof nsIComponentRegistrar);
>-  // Previous do_check_true() is not a test check.
>+  let Cm = Components.manager;
>+
>+  ok(Cm instanceof Components.interfaces.nsIComponentRegistrar,
>+     "[do_load_module()] instanceof nsIComponentRegistrar", Cm);

Why this third argument again?


>@@ -330,8 +377,56 @@ function do_parse_document(aPath, aType)
>   var parser = Components.classes[parserClass]
>                          .createInstance(C_i.nsIDOMParser);
>   var doc = parser.parseFromStream(stream, null, lf.fileSize, aType);
>   parser = null;
>   stream = null;
>   lf = null;
>   return doc;
> }
>+
>+function is(aLeft, aRight, aText, aStack) {
>+  if (!aStack)
>+    aStack = Components.stack.caller;
>+
>+  ok(aLeft == aRight, aText, "Got " + aLeft + ", expected " + aRight + ".",
>+     aStack);
>+}

Recognizing that this might make for more effort in porting, I argue we should make (todo)?is(not)? (but not ok or todo -- I have no problem with their implicit convert-to-boolean semantics) use same-value semantics as defined by ES5, *not* loose-equality semantics.  The main difference is that this would require explicit conversions when comparing values of different types.  It would differ from ===, however, in that NaN would be the same as NaN (NaN is not equal to any value, even NaN, by == or ===) and that +0 is not the same as -0.  The SameValue algorithm could be written as follows:

function _sameValue(a, b)
{
  if (typeof a === "number" && typeof b === "number")
  {
    if (a !== a && b !== b)
      return true;
    if (a === 0 && b === 0)
      return 1 / a === 1 / b;
  }
  return a === b;
}

Please use this in the implementation of the test methods rather than loose equality or inequality.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jmaher, could you have a look please?
For more information, please visit auto_nag documentation.