Closed
Bug 1122909
Opened 9 years ago
Closed 9 years ago
Show clearer error message in jstests.py when shell could not be found at the given path
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: ProgramFOX, Assigned: ProgramFOX)
Details
Attachments
(1 file, 4 obsolete files)
1.59 KB,
patch
|
ProgramFOX
:
review+
|
Details | Diff | Splinter Review |
When you pass a shell path to jstests.py that's not valid, it gives an error message that does not clearly tell you that the entered path is invalid. This has confused me some times, so I created this patch, which shows a clear error message when the shell could not be found.
Assignee | ||
Updated•9 years ago
|
Attachment #8550710 -
Flags: review?(terrence)
Comment 1•9 years ago
|
||
Comment on attachment 8550710 [details] [diff] [review] Patch - show clear error message in jstests.py when shell could not be found Review of attachment 8550710 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8550710 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 2•9 years ago
|
||
Added r=terrence to the commit message, and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e095744a3228
Attachment #8550710 -
Attachment is obsolete: true
Attachment #8550851 -
Flags: review+
Assignee | ||
Comment 3•9 years ago
|
||
The try push busted. Seems like there are cases where it's valid if options.js_shell is None, so this gave a TypeError when passing None to isfile. I added a None check.
Attachment #8550851 -
Attachment is obsolete: true
Attachment #8550861 -
Flags: review?(terrence)
Comment 4•9 years ago
|
||
Comment on attachment 8550861 [details] [diff] [review] Patch - show clear error message in jstests.py when shell could not be found Review of attachment 8550861 [details] [diff] [review]: ----------------------------------------------------------------- Right, I should have remembered that we don't pass a shell for manifest expansion. Good find.
Attachment #8550861 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Added r=terrence to the patch. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b72894f7070c
Attachment #8550861 -
Attachment is obsolete: true
Attachment #8551933 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
The Mac static analysis on Try couldn't run because I pushed my patch on top of a stale m-c clone [0]; I pulled, qrefreshed, exported and uploaded the patch again (code changes weren't necessary). New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aa7436331cc [0]: http://logs.glob.uno/?c=mozilla%23developers#c1144303
Attachment #8551933 -
Attachment is obsolete: true
Attachment #8552513 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
The busted build in the Try server worked after restarting it, and the only failed test on Win7 is caused by bug 1123404. I believe this can be checked in, adding checkin-needed.
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → programfox
Status: NEW → ASSIGNED
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a913718a2f80
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a913718a2f80
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•