Closed Bug 1240550 Opened 4 years ago Closed 4 years ago

JavaScriptError should check for fnName and file, not line

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(1 file)

The nested if clause to compute the trace message for JavaScriptError should check the fnName and file variables as entry requirements, not the line variable.

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/error.js?from=marionette%2Ferror.js#186
Assignee: nobody → ato
Status: NEW → ASSIGNED
Previously fnName and line was tested as the entry requirement for
printing the filename to the trace information.  Testing line here was
premature since it is meant to be an optional argument.

This patch rectifies this behaviour by testing for each of the optional
arguments sequentially.  This means the file argument is required to print
the line, and the fnName argument is required to print any of those two.

Review commit: https://reviewboard.mozilla.org/r/31289/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31289/
Attachment #8709130 - Flags: review?(dburns)
Comment on attachment 8709130 [details]
MozReview Request: Bug 1240550 - Make fnName, file, and line optional arguments; r?automatedtester

https://reviewboard.mozilla.org/r/31289/#review28063
Attachment #8709130 - Flags: review?(dburns) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c66efbc395 for xpcshell testing/marionette/test_error.js | test_JavaScriptError - [test_JavaScriptError : 152] "funcname @file, line line" == "funcname@file, line line" like https://treeherder.mozilla.org/logviewer.html#?job_id=20007366&repo=mozilla-inbound
This appears to have been caused by a race condition for the brief moment bug 1239373 was on inbound at the same time as this patch.  It should be fine to re-land this changeset as-is now that bug 1239373 has been backed out.
Comment on attachment 8709130 [details]
MozReview Request: Bug 1240550 - Make fnName, file, and line optional arguments; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31289/diff/1-2/
This is an example where the first xpcshell tests for Marionette actually caught a real bug!  I pushed an update to this patch that adds a whitespace character.
https://hg.mozilla.org/mozilla-central/rev/c0b8624a9591
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.