1036612 - mozafterpaint is used for a test when a previous test uses it

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in our talos jobs we run a series of tests- they are all meant to be independent with custom preferences, profiles, and browser sessions.  For some reason when one of the tests sets tpmozafterpaint=True, all the other tests use it as well.

Comment 1

[Avi Halachmi (:avih)]

Great catch!

Could you please elaborate how you concluded this? (e.g. do you look for something explicit at the full log? etc)

This _might_ also explain why on bug 1027481 we don't set mozAfterPaint (at test.py), and yet the test results look as if mAP was set.

Once this bug is fixed, this bug might mean that we'll have to list all the tests which _don't_ have mAP set at test.py, but _do_ have it set indirectly from one of the previous tests at this run group and.. well.. do something about them :)

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: allow mozafterpaint to be per test (1.0)

Comment 3

[Avi Halachmi (:avih)]

Just some more info, according to Joel, currently no test is affected by this issue.

This patch only makes the system more robust against future cases which would have been affected by this issue.

Comment 4

[Dan Minor [:dminor]]

Comment on attachment 8453808 [details] [diff] [review]
allow mozafterpaint to be per test (1.0)

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

Looks good to me.

::: talos/test.py
@@ +410,5 @@
>  
>      tpscrolltest = True
>      """ASAP mode"""
> +    tpmozafterpaint = False
> +    preferences = {'layout.frame_rate': 0, 

nit: trailing whitespace

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the review!
https://hg.mozilla.org/build/talos/rev/35f721c215f6

Comment 6

[Avi Halachmi (:avih)]

Joel, I'd appreciate some explanation of the approach to the patch here.

You seem to explicitly set tpmozafterpaint and the pref dom.send_after_paint_to_content to False on many places.

- How many tests DO use MozAfterPaint? (AFAIK, only tp5*, no?)

- Why do we need to explicitly set tpmozafterpaint = False on each test which doesn't use it when it's not defined at the base class PageloaderTest to begin with?

- Why do we need to set the pref to False on each test which doesn't use MozAfterPaint, while it's never set to True for any test (including those which use MAP) and while it's False by default in Firefox?

The way I'd imagine it:

1. The more common option should be the default within test.py, and only cases where we need non-default config should be explicitly specified.

2. The pageloader addon should just use or not use the MozAfterPaint event depending only on the configuration switch tpmozafterpaint. If it needs to set a pref to help with that (e.g. set the pref dom.send_after_paint_to_content = True), then so be it, but I'd think it should be within the domain of the pageloader addon, assuming of course that this specific pref can be changed and take effect while Firefox runs.

The reason I'm asking is that I use MozAfterPaint at bug 1078254 from within TART (new tests to measure newtab load times), but the fact that you disable this pref for all pageloader tests where the pageloader addon doesn't need to measure MozAfterPaint may interfere with this.

The other reason is that once I've noticed this, it seems it could be cleaner and more readable if common options and prefs don't have to be redeclared for many tests (with test.py).

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Avi,  The system we have for toggling variables on/off work great until we run multiple tests at once (as we do in a talos job).  I find more and more that either our test definitions or the output processing has issues with some attribute of a previous test.

A real solution would be to split the test configs out per test suite and do results uploading when each specific suite completes.