546756 - xpcshell tests should fail when child scripts generate syntax errors

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Frederic Plourde (:fred23)]

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.

Comment 1

[Frederic Plourde (:fred23)]

Attachment: 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?

Comment 2

[Frederic Plourde (:fred23)]

Attachment: patch v.2

take a look at this one instead, thanks.

Comment 3

[Jason Duell]

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.

Comment 4

[Frederic Plourde (:fred23)]

Attachment: patch v.3

updated patch per review comment.

Comment 5

[Frederic Plourde (:fred23)]

Pushed only to /projects/electrolysis because this is not an m-c issue
http://hg.mozilla.org/projects/electrolysis/rev/0b53086e8a2f3693c929f78aefba224bf2eef888

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Frederic Plourde (:fred23)]

(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.