1343129 - Taskcluster external media tests are not returning error codes in case of failure

Status: RESOLVED FIXED

(Testing Graveyard :: external-media-tests, defect)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

As noted in Bug 1342449, the task cluster tests are not correctly setting their return code. This results in failures not correctly being picked up by the test systems.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

Right now the base run_media_tests function doesn't set the self.return_code
value based on the result of the job result parser. Overrides of the function
appear to need to set this value if it's to be used.

This changeset changes that so that the base class now sets the value.
Overrides are free to alter the behaviour so that the value can be set as
appropriate depending on the tests, but removes the ability to footgun by
assuming that the base will set the value (as I did).

Review commit: https://reviewboard.mozilla.org/r/115938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115938/

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8841848 [details]
Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

https://reviewboard.mozilla.org/r/115938/#review117414

r+wc

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:315
(Diff revision 1)
> +        self.return_code = return_code
>          return self.job_result_parser.status

Right now `self.return_code` is based just on the return code of the process launched by `run_command`. To be based on the parser you want: `self.return_code = self.job_result_parser.status`

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8841848 [details]
Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

https://reviewboard.mozilla.org/r/115938/#review117414

> Right now `self.return_code` is based just on the return code of the process launched by `run_command`. To be based on the parser you want: `self.return_code = self.job_result_parser.status`

I believe return code expects to be an int, is there a convention for conversion between the status string and integer return codes? I see buildbot.py contains an EXIT_STATUS_DICT, would it be appropriate to use something like this?

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8841848 [details]
Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

https://reviewboard.mozilla.org/r/115938/#review117414

> I believe return code expects to be an int, is there a convention for conversion between the status string and integer return codes? I see buildbot.py contains an EXIT_STATUS_DICT, would it be appropriate to use something like this?

True! My bad. Yes, that would be good and in fact that's was self.buildbot_status does. I think this really boils down to just consolidating the run_media_tests method from FirefoxMediaTestsBuildbot,. This can all go in the base: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/firefox_media_tests_buildbot.py#47-55

Sorry for missing that earlier.

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8841848 [details]
Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

https://reviewboard.mozilla.org/r/115938/#review117414

> True! My bad. Yes, that would be good and in fact that's was self.buildbot_status does. I think this really boils down to just consolidating the run_media_tests method from FirefoxMediaTestsBuildbot,. This can all go in the base: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/firefox_media_tests_buildbot.py#47-55
> 
> Sorry for missing that earlier.

Cool, I'll get that sorted and this updated. Appreciate the help!

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8841848 [details]
Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115938/diff/1-2/

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8841848 [details]
Bug 1343129 - Update the base media test class to set self.return_code based on the job result parser.

https://reviewboard.mozilla.org/r/115938/#review117414

> Cool, I'll get that sorted and this updated. Appreciate the help!

Updated. Let me know if this looks sensible.

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

lgtm

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b557bdc314e7
Update the base media test class to set self.return_code based on the job result parser. r=maja_zf

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Resolving: I'm now seeing failures reported correctly in TH, looks like landing this has had the desired effect.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

FYI bugs will be marked as fixed once the patch gets merged into mozilla-central by the sheriffs. Reopening until this happened.

Comment 12

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

It does appear to have been merged.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Indeed: https://hg.mozilla.org/mozilla-central/rev/b557bdc314e7

Interesting that there was no comment from pulsebot about that landing on mozilla-central. Maybe there was a temporary outage again. So we can close it.