Closed
Bug 729361
Opened 13 years ago
Closed 13 years ago
buildRemoteManifest silently succeeds on failure; it should fail
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Unassigned)
Details
Attachments
(2 files)
1.77 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
If buildRemoteManifest is called with a file that doesn't exist or
other baddities, we silently return the name of the origin manifest:
http://hg.mozilla.org/build/talos/file/caf24bd3f566/talos/PerfConfigurator.py#l325
Instead, we should probably fail.
Also, we use
`manifestData = fHandle.read()
...
for line in manifestData.split('\n'):`
vs
manifestLines = fHandle.readlines():
...
for line in lines
Reporter | ||
Comment 1•13 years ago
|
||
Attachment #599417 -
Flags: review?(jmaher)
Comment 2•13 years ago
|
||
Comment on attachment 599417 [details] [diff] [review]
err out harder
Review of attachment 599417 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with a better message for raise.
::: talos/PerfConfigurator.py
@@ +324,5 @@
> fHandle.close()
> except:
> if fHandle:
> fHandle.close()
> + raise
raise what, your hands? Some kind of message is always nice.
Attachment #599417 -
Flags: review?(jmaher) → review+
Reporter | ||
Updated•13 years ago
|
Whiteboard: [talos-checkin-needed]
Reporter | ||
Comment 3•13 years ago
|
||
Not sure what level of verbosity you want here. Re-raising the current exception will print the error to screen, such as IOError: [Errno 2] No such file or directory: 'dfjasdfk' for a missing file
Attachment #606623 -
Flags: review?(jmaher)
Comment 4•13 years ago
|
||
Comment on attachment 606623 [details] [diff] [review]
add a comment
Review of attachment 606623 [details] [diff] [review]:
-----------------------------------------------------------------
well, there is no perfect method for error handling, so this will do.
Attachment #606623 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 5•13 years ago
|
||
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=33e5d14f7a1f
tested on android locally with ts and tsvg; works fine
Reporter | ||
Comment 6•13 years ago
|
||
looks pretty green, pushing
Reporter | ||
Comment 7•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 8•13 years ago
|
||
Try run for 33e5d14f7a1f is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=33e5d14f7a1f
Results (out of 75 total builds):
success: 68
failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-33e5d14f7a1f
Updated•12 years ago
|
Whiteboard: [talos-checkin-needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•