Closed Bug 546756 Opened 13 years ago Closed 13 years ago

xpcshell tests should fail when child scripts generate syntax errors

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: fred23, Assigned: fred23)

References

Details

(Whiteboard: IPC)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v.1 (obsolete) — Splinter Review
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?
Attachment #427475 - Flags: review?(jduell.mcbugs)
Attached patch patch v.2 (obsolete) — Splinter Review
take a look at this one instead, thanks.
Attachment #427475 - Attachment is obsolete: true
Attachment #427476 - Flags: review?(jduell.mcbugs)
Attachment #427475 - Flags: review?(jduell.mcbugs)
Assignee: nobody → bugzillaFred
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+
Attached patch patch v.3Splinter Review
updated patch per review comment.
Attachment #427476 - Attachment is obsolete: true
Attachment #429745 - Flags: review+
Pushed only to /projects/electrolysis because this is not an m-c issue
http://hg.mozilla.org/projects/electrolysis/rev/0b53086e8a2f3693c929f78aefba224bf2eef888
Status: NEW → RESOLVED
Closed: 13 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.