Bug 521922's patch (Patch for running xpcshell tests cleanly in electrolysis in either content or chrome processes) does a great job, but there's a problem. When the child test script generates a "SyntaxError:" error output, runxpcshelltests.py doesn't detect it and hence, consider the test as passed. This is not appropriate as we really should fail here.
Created attachment 427475 [details] [diff] [review] patch v.1 I investigated this, and the reason is quite simple. in head.js::_execute_test(), run_test *DOES NOT* throw when run_test has a syntax error. It throws for a lot of cases, e.g. reference errors, but not for syntax... I'm not sure why. Hence, it doesn't output TEST-UNEXPECTED-FAIL, which, in turn, goes unnoticed in xpcshelltests.py. the test passes, but shouldn't. What could be used for syntax error detections by xpcshelltests.py is the "SyntaxError:" keyword that writes to the child output. That's what I'm doing in this patch. Now, only question is : There might be some tests that output "SyntaxError:" on purpose, but that seems *really* unlikely to me. What should we do with that?
Created attachment 427476 [details] [diff] [review] patch v.2 take a look at this one instead, thanks.
Comment on attachment 427476 [details] [diff] [review] patch v.2 This looks good. Thanks for figuring this out. Let's change the regex to search for ": SyntaxError: ": it looks like we always get the leading colon and space, and that ought to make it nearly impossible that some random test output would match.
Attachment #427476 - Flags: review?(jduell.mcbugs) → review+
Created attachment 429745 [details] [diff] [review] patch v.3 updated patch per review comment.
Pushed only to /projects/electrolysis because this is not an m-c issue http://hg.mozilla.org/projects/electrolysis/rev/0b53086e8a2f3693c929f78aefba224bf2eef888
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Just FWIW, jduell isn't really a peer on the test harness code, so I'd appreciate it if you did get peer review on patches to the test harnesses. This is a pretty minor change, and it's only in e10s, so this particular case is not a big deal.
Component: XPConnect → XPCShell Harness
Product: Core → Testing
QA Contact: xpconnect → xpcshellharness
(In reply to comment #7) > Just FWIW, jduell isn't really a peer on the test harness code, so I'd > appreciate it if you did get peer review on patches to the test harnesses. This > is a pretty minor change, and it's only in e10s, so this particular case is not > a big deal. k, I'll do next time, for sure. thanks.
You need to log in before you can comment on or make changes to this bug.