1075787 - Gij failures are currently masked and reported as successful

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Kevin Grandon :kgrandon]

Since splitting Gij, it seems that failures can currently be hidden.

I'm seeing runs like this: https://tbpl.mozilla.org/?tree=B2g-Inbound&showall=1&rev=cdc2bda1a15c

Clicking on the 'Gij' result will clearly show test failures, but yet it's reported as green. We need to fix this asap otherwise we may quickly regress a lot of tests. We may consider closing gaia in the meantime.

Comment 1

[Jonathan Griffin (:jgriffin)]

This problem occurs because mozharness uses the exit code of the process to determine the overall pass/failed status, and apparently Gij always exits with 0.

We can fix this on the mozharness side by making it take the number of passed/failed tests into account.

Comment 2

[Jonathan Griffin (:jgriffin)]

Attachment: Make Gij take passed/failed test counts into account when setting tbpl status,

Comment 3

[Jonathan Griffin (:jgriffin)]

When this lands, it will cause Gij to go perma-orange, unless the currently failing tests are fixed or hidden by the time this happens.

Comment 4

[Kevin Grandon :kgrandon]

Thanks Jonathan! I'm currently working my way through the perma-failures. Perma-orange is much more preferable to us than the masked failures. Thanks for knocking this out so quickly.

Comment 5

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

Comment on attachment 8498407 [details] [diff] [review]
Make Gij take passed/failed test counts into account when setting tbpl status,

Review of attachment 8498407 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozharness/mozilla/testing/gaia_test.py
@@ +251,4 @@
>          """
>          pass
>  
> +    def publish(self, code, passed=None, failed=None):

Does this ever get called without passed/failed? If not, might as well make them required parameters.

Comment 6

[Jonathan Griffin (:jgriffin)]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Comment on attachment 8498407 [details] [diff] [review]
> Make Gij take passed/failed test counts into account when setting tbpl
> status,
> 
> Review of attachment 8498407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozharness/mozilla/testing/gaia_test.py
> @@ +251,4 @@
> >          """
> >          pass
> >  
> > +    def publish(self, code, passed=None, failed=None):
> 
> Does this ever get called without passed/failed? If not, might as well make
> them required parameters.

It does...i.e., by gaia_unit tests (which don't need it, since they exit with the appropriate exit code).

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8498407 [details] [diff] [review]
Make Gij take passed/failed test counts into account when setting tbpl status,

https://hg.mozilla.org/build/mozharness/rev/7030a62bf270

I'm going to retrigger on cedar to make sure this doesn't have any unintended side-effects before pushing to production.

Comment 8

[Jonathan Griffin (:jgriffin)]

Retrigger was good, pushed to production: https://hg.mozilla.org/build/mozharness/rev/244abc6524fa

Comment 9

[Ed Morley [:emorley]]

We should also fix the harness returning zero if there were failures; ideally we'll move away from manually checking test run counts in the future. I'll file another bug for this.

Moving to releng, since TBPL only displays the results given to it by buildbot, and as such this was fixed by a mozharness patch (though curious how the changes in bug 1046694 made us regress this behaviour?).

Comment 10

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #9)
> We should also fix the harness returning zero if there were failures;
> ideally we'll move away from manually checking test run counts in the
> future. I'll file another bug for this.
> 
> Moving to releng, since TBPL only displays the results given to it by
> buildbot, and as such this was fixed by a mozharness patch (though curious
> how the changes in bug 1046694 made us regress this behaviour?).

Comment 11

[Jonathan Griffin (:jgriffin)]

(In reply to Ed Morley [:edmorley] from comment #9)
> We should also fix the harness returning zero if there were failures;
> ideally we'll move away from manually checking test run counts in the
> future. I'll file another bug for this.
> 
> Moving to releng, since TBPL only displays the results given to it by
> buildbot, and as such this was fixed by a mozharness patch (though curious
> how the changes in bug 1046694 made us regress this behaviour?).

I've looked at bug 1046694 and don't believe it caused this regression.  More likely, Gij used to return a non-zero exit code on failure, but at some point stopped doing that.

Comment 12

[Ed Morley [:emorley]]

Ah I was basing that from the timing and also the OP blocking bug 1067892 (which was the TBPL part for that bug).

Comment 13

[Kevin Grandon :kgrandon]

I think that some of the work that went into splitting the Gij test suite surfaced this. I'm not sure exactly which bug surfaced it though. Apologies if I was wrong.

Comment 14

[Chris Cooper [:coop] (he/him)]

Merged to production, and deployed.