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!

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?

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.

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

[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?

Av1, revised and unbitrotted.

jwalden, ping for review ?

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.