Closed
Bug 819033
Opened 10 years ago
Closed 10 years ago
Native support for Task-based tests in xpcshell runner
Categories
(Testing :: XPCShell Harness, defect)
Testing
XPCShell Harness
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla20
People
(Reporter: gps, Assigned: gps)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 3 obsolete files)
9.24 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
2.99 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
Task.jsm and it's main function Task.spawn() are friggin awesome, especially for unit tests where you are often chaining lots of async, promise-based APIs together. I found myself writing a lot of boilerplate in my xpcshell tests to deal with Task.spawn so failures would be handled properly. So, I thought "why not add this to the xpcshell runner itself." So, I did. The attached patch adds a new function, add_task_test(). Conceivably, I could probably hack around things and overload add_test() to accept generator functions. But, I believe that would involve some generator light magic. I think the approach I took is simplest. While I was here, I also gave add_test() some basic test coverage. But only a little. The only nit I have with the implementation is that a do_check_* failure inside the generator function yields an exceptionally long stack stack. There are a lot of frames from Task.jsm and promise's core.js in there. But, the main failure is still on top of the stack, so it's easy to see what went wrong. I would be happy with a follow-up bug to sanitize the stack trace before printing. Ted gets primary review because this is all xpcshell runner code. I'd like Yoric and rnewman to offer feedback on the implementation and utility of this. I'd like to write the unit tests for bug 813833 using this. So, a review within the next week or two would be appreciated.
Attachment #689334 -
Flags: review?(ted)
Attachment #689334 -
Flags: feedback?(rnewman)
Attachment #689334 -
Flags: feedback?(dteller)
Comment 1•10 years ago
|
||
Comment on attachment 689334 [details] [diff] [review] Add add_task_test() to xpcshell runner, v1 Review of attachment 689334 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/xpcshell/head.js @@ +884,5 @@ > + * > + * Unlike add_test(), there is no need to call run_next_test(). The next test > + * will run automatically as soon the task function is exhausted. To trigger > + * premature (but successful) termination of the function, throw a Task.Result > + * instance. Add an example of this. @@ +918,5 @@ > if (gTestIndex < gTests.length) { > do_test_pending(); > + let isTask; > + [isTask, gRunningTest] = gTests[gTestIndex++]; > + print("is task? " + isTask); Kill this. Fold it into the next line if you want. @@ +923,5 @@ > + print("TEST-INFO | " + _TEST_FILE + " | Starting " + gRunningTest.name); > + > + if (isTask) { > + Task.spawn(gRunningTest).then(run_next_test, function onError (error) { > + do_throw(error); Just Task.spawn(gRunningTest).then(run_next_test, do_throw);
Attachment #689334 -
Flags: feedback?(rnewman) → feedback+
Comment 2•10 years ago
|
||
Comment on attachment 689334 [details] [diff] [review] Add add_task_test() to xpcshell runner, v1 Paolo was already integrating something similar in Places, I think he may be interested in feedbacking this. fwiw we were using just add_task() without a _test suffix
Attachment #689334 -
Flags: feedback?(paolo.mozmail)
Assignee | ||
Comment 3•10 years ago
|
||
Incorporated feedback. (I can't believe I left that print statement in there.)
Assignee: nobody → gps
Attachment #689334 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #689334 -
Flags: review?(ted)
Attachment #689334 -
Flags: feedback?(paolo.mozmail)
Attachment #689334 -
Flags: feedback?(dteller)
Attachment #689349 -
Flags: review?(ted)
Attachment #689349 -
Flags: feedback?(dteller)
Comment 4•10 years ago
|
||
I will take a look at the patch once I have popped a few patches from my review queue. Just one bit on the intention: I have somewhere in my code a function |TaskUtils.spawn| that behaves exactly as |Task.spawn| but also reports errors using |Components.utils.reportError|. Wouldn't it be better to split this bug in two? One for error reporting and one for native support?
Assignee | ||
Comment 5•10 years ago
|
||
This patch merely allows task functions to be registered as tests. There is nothing to it besides making it easier to run tasks as tests. I don't think there is anything to split. If you create a utility function that does more intelligent error handling, the code in this patch could conceivably switch to use that.
Comment 6•10 years ago
|
||
if you check the head_common changes here https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=763295&attachment=683479 Paolo used a different way to distinguish tasks from functions, basically add_task() creates a wrapper function, so that gTests doesn't require modifications
Comment 7•10 years ago
|
||
Comment on attachment 689349 [details] [diff] [review] Add add_task_test() to xpcshell runner, v2 Review of attachment 689349 [details] [diff] [review]: ----------------------------------------------------------------- I don't really know anything about Task.jsm, but the harness changes look good. Thanks for adding tests to selftest.py! ::: testing/xpcshell/head.js @@ +901,5 @@ > + * add_task_test(function test_early_return() { > + * let result = yield somethingThatReturnsAPromise(); > + * > + * if (!result) { > + * throw Task.Result(); Can you explain with a meta-comment what this actually does?
Attachment #689349 -
Flags: review?(ted) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Marco Bonardo [:mak] (Away 10-11 Dec) from comment #6) > if you check the head_common changes here > https://bugzilla.mozilla.org/page.cgi?id=splinter. > html&bug=763295&attachment=683479 > Paolo used a different way to distinguish tasks from functions, basically > add_task() creates a wrapper function, so that gTests doesn't require > modifications I could do that, sure. I don't think it makes much difference. I agree having one path for test execution is simpler. Although, it does introduce an extra stack frame. I think the meh principle applies. Ted, et al: do you like add_task_test or add_task? We could even alias them if we wanted!
Comment 9•10 years ago
|
||
I don't have a preference. Use your judgement, but pick just one please.
Comment 10•10 years ago
|
||
Comment on attachment 689349 [details] [diff] [review] Add add_task_test() to xpcshell runner, v2 Review of attachment 689349 [details] [diff] [review]: ----------------------------------------------------------------- We went for add_task in Places (after trying calling it add_test_task) because: * It's shorter, so it's easier to give clearer names to test functions and still stay under 80 characters. * Usually, you already write "test" in the function, like add_task(function test_something() { ... }). * You don't have to remember whether it's add_test_task or add_task_test. ::: testing/xpcshell/head.js @@ +864,5 @@ > * @return the test function that was passed in. > */ > let gTests = []; > function add_test(func) { > + gTests.push([false, func]); I like defining whether the function is a task explicitly, like this, more than using a wrapper function like I had to do in Places to avoid modifying the harness. Unfortunately, some tests populate gTests directly. I think we should either go with the wrapper function approach, or rename gTests to something else, so that it becomes private to the harness, then search the tree for direct uses of gTests, and fix them to use add_test instead. A tryserver build with this patch applied will show those uses. @@ +869,5 @@ > return func; > } > > +// We lazy import Task.jsm so we don't incur a run-time penalty for all tests. > +let Task; Variables defined here become available to tests, so this should be renamed to something that cannot accidentally conflict with variables defined in test files. @@ +886,5 @@ > + * will run automatically as soon the task function is exhausted. To trigger > + * premature (but successful) termination of the function, throw a Task.Result > + * instance. > + * > + * Example usage: You wrote better examples than mine :-) Something I would mention in the comment, taken from my patch: Test files should call run_next_test() inside run_test() to execute all the asynchronous tests, as usual. Test files may include both function added with add_test() as well as function added with add_task(). @@ +901,5 @@ > + * add_task_test(function test_early_return() { > + * let result = yield somethingThatReturnsAPromise(); > + * > + * if (!result) { > + * throw Task.Result(); You can just call "return" in a generator, if you don't return a value. Since we ignore any result of the task, just "return" here would be clearer. @@ +909,5 @@ > + * }); > + */ > +function add_task_test(func) { > + if (!Task) { > + Task = Components.utils.import("resource://gre/modules/Task.jsm").Task; This will import the module in the global scope; you probably want to import to a private scope specified as the second argument of Cu.import. Tests that want to call Task.spawn would have to define their own module lazy getter. @@ +931,5 @@ > + [isTask, gRunningTest] = gTests[gTestIndex++]; > + print("TEST-INFO | " + _TEST_FILE + " | Starting " + gRunningTest.name); > + > + if (isTask) { > + Task.spawn(gRunningTest).then(run_next_test, do_throw); You probably want to call do_report_unexpected_exception rather than do_throw so that you get the stack trace of the place where the exception was thrown.
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Paolo Amadini [:paolo] from comment #10) Great feedback! > Unfortunately, some tests populate gTests directly. I think we should either > go with the wrapper function approach, or rename gTests to something else, > so that it becomes private to the harness, then search the tree for direct > uses of gTests, and fix them to use add_test instead. Wat?! I will search for and eradicate all such abuses! > @@ +869,5 @@ > > return func; > > } > > > > +// We lazy import Task.jsm so we don't incur a run-time penalty for all tests. > > +let Task; > > Variables defined here become available to tests, so this should be renamed > to something that cannot accidentally conflict with variables defined in > test files. Another reason why I want to significantly refactor the xpcshell runner logic so most things are defined as modules, not #included files. But, that's for another bug.
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=715f351fbf38
Assignee | ||
Comment 13•10 years ago
|
||
Incorporated feedback. Re-requesting review because I _-prefixed gTests and friends.
Attachment #689349 -
Attachment is obsolete: true
Attachment #689349 -
Flags: feedback?(dteller)
Attachment #690113 -
Flags: review?(ted)
Assignee | ||
Comment 14•10 years ago
|
||
This is what I found in a local test run. Try may find some more.
Attachment #690116 -
Flags: review?(mak77)
Assignee | ||
Comment 15•10 years ago
|
||
Sorry - forgot to qref.
Attachment #690116 -
Attachment is obsolete: true
Attachment #690116 -
Flags: review?(mak77)
Attachment #690117 -
Flags: review?(mak77)
Comment 16•10 years ago
|
||
I've just landed on inbound the patch that defines the local add_task in Places. Sorry for the race, but it was at the top of a queue of 9 bugs that has already waited for too long, and was getting bitrotten already! https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=94d11f7d9114 The upside is that these changes might have already fixed some instances of direct use of the gTests variable.
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Paolo Amadini [:paolo] from comment #16) > I've just landed on inbound the patch that defines the local add_task in > Places. Well, if you define that function in a test file, it will just override what's in head.js. So, no harm done. I think.
Comment 18•10 years ago
|
||
I'm sorry but I'm away for a couple days, so I won't be able to review these at least untile Wed. feel free to ask paolo, or land whatever is needed on top of the current tests. I just care they keep passing, rs=me if that is true.
Comment 19•10 years ago
|
||
Comment on attachment 690113 [details] [diff] [review] Add add_task() to xpcshell runner, v3 Review of attachment 690113 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/xpcshell/head.js @@ +862,5 @@ > * async tests. > * > * @return the test function that was passed in. > */ > +let _gTests = []; I approve. I didn't know we had tests touching this directly. :-/
Attachment #690113 -
Flags: review?(ted) → review+
Comment 20•10 years ago
|
||
(In reply to Paolo Amadini [:paolo] from comment #10) > We went for add_task in Places (after trying calling it add_test_task) > because: [...] > * You don't have to remember whether it's add_test_task or add_task_test. This is a great reason, I am totally in favor.
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 690117 [details] [diff] [review] Part 0: Remove references to private test runner variables from tests, v2 test_486978_sort_by_date_queries.js was bit rotted. But, the one-liner from test_sql_guid_functions.js should be good for review.
Attachment #690117 -
Flags: review?(mak77) → review?(paolo.mozmail)
Comment 22•10 years ago
|
||
Comment on attachment 690117 [details] [diff] [review] Part 0: Remove references to private test runner variables from tests, v2 Review of attachment 690117 [details] [diff] [review]: ----------------------------------------------------------------- I missed this easy review yesterday. Thanks for fixing these tests! ::: toolkit/components/places/tests/unit/test_sql_guid_functions.js @@ +13,5 @@ > * The guid to check. > */ > function check_invariants(aGuid) > { > + print("Checking guid '" + aGuid + "'"); You can use "do_print" here.
Attachment #690117 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 23•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/907a846d368f https://hg.mozilla.org/integration/mozilla-inbound/rev/fac650c31656
Keywords: dev-doc-needed
Target Milestone: --- → mozilla20
Comment 24•10 years ago
|
||
Sorry had to backout for: { TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/head_bookmarks.js -> file:///builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/head_common.js | [remove_bookmarks_html : 487] false == false TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/head_bookmarks.js -> file:///builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/head_common.js | [remove_all_JSON_backups : 543] false == false ###!!! ASSERTION: The statement 'SELECT h.id, h.url, IFNULL(b.title, h.title), h.rev_host, h.visit_count, h.last_visit_date, f.url, null, b.id, b.dateAdded, b.lastModified, b.parent, null, h.frecency, b.position, b.type, b.fk, b.guid FROM moz_bookmarks b LEFT JOIN moz_places h ON b.fk = h.id LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE b.parent = :parent ORDER BY b.position ASC' failed to compile with the error message ''.: 'Error', file ../../../dist/include/mozilla/storage/StatementCache.h, line 125 imgLoader::SupportImageWithMimeType(char const*)+0x00989282 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x00C3F802] JSD_GetValueForObject+0x0007D794 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x014D8D84] JSD_GetValueForObject+0x000799CE [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x014D4FBE] NS_InvokeByIndex_P+0x0000021D [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x019A796D] xpc_LocalizeContext(JSContext*)+0x00019D06 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01162976] xpc_LocalizeContext(JSContext*)+0x00017B3A [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x011607AA] xpc_LocalizeContext(JSContext*)+0x00021BB1 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0116A821] JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0003022C [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209FD6C] JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0002B48C [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209AFCC] JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x00027CD3 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x02097813] JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x00020475 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0208FFB5] JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0002B460 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209AFA0] JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0002BBCE [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209B70E] JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*)+0x0000014B [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01FF7ACB] xpc_LocalizeContext(JSContext*)+0x0000F3FA [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0115806A] xpc_LocalizeContext(JSContext*)+0x00007402 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01150072] nsXPTCStubBase::Stub249()+0x00000315 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x019A8C95] ###!!! ASSERTION: The statement 'SELECT h.id, h.url, IFNULL(b.title, h.title), h.rev_host, h.visit_count, h.last_visit_date, f.url, null, b.id, b.dateAdded, b.lastModified, b.parent, null, h.frecency, b.position, b.type, b.fk, b.guid FROM moz_bookmarks b LEFT JOIN moz_places h ON b.fk = h.id LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE b.parent = :parent ORDER BY b.position ASC' failed to compile with the error message ''.: 'Error', file ../../../dist/include/mozilla/storage/StatementCache.h, line 125 <<<<<<< } https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fac650c31656 https://hg.mozilla.org/integration/mozilla-inbound/rev/469076f776d6
Assignee | ||
Comment 25•10 years ago
|
||
Looks like I missed a test that uses gTests. (I saw this in the try run but figured it was an intermittent crash.) This Try push instruments the test runner to abort tests if gTests is being defined directly by a test. https://tbpl.mozilla.org/?tree=Try&rev=1f5cabaa0a52
Assignee | ||
Comment 26•10 years ago
|
||
Try was green. Only change from last review was a trivial change to /browser/components/places/tests/unit/test_browserGlue_prefs.js to not use gTests. I consider this not worthy of time for review. https://hg.mozilla.org/integration/mozilla-inbound/rev/7592c22f19ec https://hg.mozilla.org/integration/mozilla-inbound/rev/fef8ecbe43f5
Comment 27•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #25) > Looks like I missed a test that uses gTests. (I saw this in the try run but > figured it was an intermittent crash.) I'm happy to take a look at cases like this in the future if that helps, just ping me on IRC. (Also there are not many cases where TBPL won't make bug suggestions now, so if there isn't a suggestion, it is a good sign it's something new)
Comment 28•10 years ago
|
||
yes, some tests were using gTests, but not by will of replacing the global one, just cause it's common to prefix g to globals and Tests was a convenient name... that is unfortunate indeed.
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7592c22f19ec https://hg.mozilla.org/mozilla-central/rev/fef8ecbe43f5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•