TPS has to exit with code != 0 in case of failures

RESOLVED FIXED in Firefox 32


5 years ago
9 months ago


(Reporter: whimboo, Assigned: cosmin-malutan)



Firefox Tracking Flags

(firefox32 fixed, firefox33 fixed, firefox34 fixed)



(1 attachment)

Right now TPS always exits with code 0 even if failures happened. This should be changed so that it exists appropriately to the testrun status. This is necessary so that jenkins can detect if the tests were successful or not.

Comment 1

5 years ago
Posted patch patch v1.0Splinter Review
Henrik requested that I would have a look over this while hes gone and that Dave can review after an internal review:

So here is the fix-patch for this, I added the exit code in the main class so we don't have to handle all different situation of single test run, group tests run etc.
Assignee: hskupin → cosmin.malutan
Attachment #8463355 - Flags: review?(dave.hunt)
Attachment #8463355 - Flags: review?(andrei.eftimie)
Comment on attachment 8463355 [details] [diff] [review]
patch v1.0

Review of attachment 8463355 [details] [diff] [review]:

::: testing/tps/tps/
@@ +116,4 @@
>                        )
>      TPS.run_tests()
> +    if TPS.numfailed > 0 or TPS.numpassed == 0:

Is it possible to skip tests? If so, we may have a situation where zero tests are run, and that might not actually indicate a failure. I think it might be best to just check if numfailed is > 0, but I'm not familiar with these tests so please convince if you think this is the best approach.
Attachment #8463355 - Flags: review?(dave.hunt) → review-

Comment 3

5 years ago
Here is the code where it switches the email address to send the notification email:

So far I haven't seen any skipped test but I think we would remove them from this list when they will fail, I'm not aware of any other skip mechanism.
I thought we might check for passed builds in case we have no test in and to be in sync with code that sends the emails. It wouldn't be ok to mark the build as PASS if we send a failure notification email. 
From my point of view we could either check only for failures and change the check for emails to or live it as it is. 
What do you think Dave?
Comment on attachment 8463355 [details] [diff] [review]
patch v1.0

Review of attachment 8463355 [details] [diff] [review]:

Okay, that's enough to convince me. Thanks.
Attachment #8463355 - Flags: review- → review+

Comment 5

5 years ago
Comment on attachment 8463355 [details] [diff] [review]
patch v1.0

Review of attachment 8463355 [details] [diff] [review]:

Given the way we handle things for email, and how this is structured it looks good to me.
Attachment #8463355 - Flags: review?(andrei.eftimie) → review+

Comment 6

5 years ago
This needs to be checked in, it doesn't need a try run as the TPS tests are ran on pulse message.
Keywords: checkin-needed
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
We also need to get this backported to aurora and beta.


9 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.