buildRemoteManifest silently succeeds on failure; it should fail

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: k0scist, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Created attachment 599417 [details] [diff] [review]
err out harder
Attachment #599417 - Flags: review?(jmaher)
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

7 years ago
Whiteboard: [talos-checkin-needed]
(Reporter)

Comment 3

7 years ago
Created attachment 606623 [details] [diff] [review]
add a comment

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

7 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

7 years ago
looks pretty green, pushing
(Reporter)

Comment 7

7 years ago
pushed: http://hg.mozilla.org/build/talos/rev/7e058f98d220
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 8

7 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
Whiteboard: [talos-checkin-needed]
You need to log in before you can comment on or make changes to this bug.