xpcshell tests should fail when child scripts generate syntax errors

RESOLVED FIXED

Status

Testing
XPCShell Harness
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: fred23, Assigned: fred23)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: IPC)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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?
Attachment #427475 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 2

8 years ago
Created attachment 427476 [details] [diff] [review]
patch v.2

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)

Updated

8 years ago
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+
(Assignee)

Comment 4

8 years ago
Created attachment 429745 [details] [diff] [review]
patch v.3

updated patch per review comment.
Attachment #427476 - Attachment is obsolete: true
Attachment #429745 - Flags: review+
(Assignee)

Comment 5

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

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Duplicate of this bug: 550721
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
(Assignee)

Comment 8

8 years ago
(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.