1423658 - Fail Windows tests early if not using default theme

Status: RESOLVED DUPLICATE of bug 1343049

(Testing :: General, defect)

Comment 1

[Geoff Brown [:gbrown]]

https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/widget/windows/nsUXThemeData.cpp#385

Comment 2

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

possible prior art in python:
https://stackoverflow.com/questions/12553675/python-pyqt-check-if-aero-enabled

Comment 3

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

working on some hacks in mozharness level:
https://hg.mozilla.org/try/rev/554aac1d745e19941962f80bef0d562902f12f2c

Comment 4

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

I have not had any luck getting results to show up on windows 7 from taskcluster.  The theme comes back as an empty string, but it works fine on my win10 box locally.  I will need to make sure this runs on win7 locally, then see if there is something we can do on a loaner.  Possibly I have different python versions or related libraries.

Comment 5

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

this script I have works on windows 10 in aws, so right now this is a windows7 specific issue.

Comment 6

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

Attachment: verify aero theme on windows

this works for the win_taskcluster_unittest.py script, possibly we want to enable this on more so wpt can benefit, I have got this working fine on try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33ae59296dedab754d6ccef4f9d4b3b885dfc236

Comment 7

[Geoff Brown [:gbrown]]

In your other try push, you are hitting 

TypeError: not all arguments converted during string formatting

...a pre-existing fault. Suggest you fix that now:

https://dxr.mozilla.org/mozilla-central/rev/457b0fe91e0d49a5bc35014fb6f86729cd5bac9b/testing/mozharness/mozharness/base/script.py#1444,1599

"...%s" % str(command)

should do it.

Comment 8

[Geoff Brown [:gbrown]]

Comment on attachment 8935816 [details] [diff] [review]
verify aero theme on windows

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

::: testing/mozharness/configs/unittests/win_taskcluster_unittest.py
@@ +281,5 @@
> +                    'mozharness', 'external_tools', 'check_win_theme.py')
> +            ],
> +            'architectures': ['32bit', '64bit'],
> +            'halt_on_failure': True,
> +            'enabled': True

Are you sure you want to run this on all windows tests? Does theme make a difference to mochitests? xpcshell??

::: testing/mozharness/external_tools/check_win_theme.py
@@ +51,5 @@
> +  SPI_GETHIGHCONTRAST = 0x0042
> +  highContrastInfo = ctypes.c_int64()
> +  retVal = ctypes.windll.user32.SystemParametersInfoA(SPI_GETHIGHCONTRAST, 0, ctypes.byref(highContrastInfo), 0)
> +  if retVal:
> +      #TODO: sIsHighContrastOn = ((highContrastInfo.dwFlags & HCF_HIGHCONTRASTON) != 0);

This TODO seems like an important omission...no?

@@ +58,5 @@
> +      return False
> +
> +def is_win8_or_later():
> +    version = platform.version()
> +    if version.startswith('6.2') or version.startswith('6.3') or version.startswith('10.0'):

These version strings are painful, I know, but I'd prefer to see something more future-proof. (If version.startswith('11.0'), it's a good bet that's win8 or later, right?)

@@ +64,5 @@
> +    else:
> +        return False
> +
> +def main():
> +    iswin8 = is_win8_or_later()

Maybe print some info to the log: "Is win8 or later? True" ... "Is high contrast? False" ...

@@ +70,5 @@
> +    # ensure we are on win7, no high contrast, and running AERO
> +    # https://searchfox.org/mozilla-central/source/widget/windows/nsUXThemeData.cpp#383
> +    if not (iswin8 and ishc):
> +        theme, color = get_theme_and_color()
> +        if theme == 'aero' and color == 'normal':

https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/widget/windows/nsUXThemeData.cpp#385 allows for aero-lite and luna...why the difference?

@@ +74,5 @@
> +        if theme == 'aero' and color == 'normal':
> +            # defaultWindowTheme!
> +            return 0
> +    # fail job
> +    print("ERROR: unable to get default theme or theme isn't Aero, we should not run tests")

Consider being more explicit: "...we should not run tests in this Windows environment (bug xxx)"

Comment 9

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

I was not able to get windows to retry, it seems that on failure we will have to have failures instead of auto retrying.  I am working to make the failures more discoverable, but overall we will see many failures here.

Comment 10

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

I did a push with --rebuild 20 and 64 chunks (read 7700 jobs), there are about 110 instances of bad OS:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db2ab6cd844886af22915f59c394aa304c67cab

While that isn't bad, I imagine this will be a 1000+ failures/week scenario if we land it.

:gbrown, do you have thoughts here?  Maybe this will remove a ton of intermittents and let us focus on this one large one.

Comment 11

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

another thought here is that if a job fails, we then detect if the theme is not aero and then fail for that reason.  That should reduce the number of failures, but it would also be near impossible to make treeherder show the correct problem and it would spend a LOT of time with running tests on these failure conditions that we can catch early on.

Comment 12

[Geoff Brown [:gbrown]]

I discussed this with jmaher today.

I would prefer to have the task auto-retry when the theme is unexpected. It sounds like the Windows taskcluster worker doesn't support retry. :(

I would prefer to fix the theme problem before adding the theme detection, so that we don't introduce a frequent intermittent failure. But it sounds like there have on-going efforts to correct the theme for a long time, and we may not be able to correct it in the near future. :(

Comment 13

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

update- as a note, I am working with :grenade in bug 1343049 to make the os theme setting more reliable.

Comment 14

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

after further discussion on this- there might be things we could do, but nothing is easy and won't be quick.

The ideal situation is to have auto-retry, but that isn't supported with the windows worker (although the GPU jobs retry...possibly that is something to look into more).

A not so ideal solution is to hack on reftest so that when we get a failure, report a default theme issue iff the theme is invalid.  That would help reduce the randomizations on the layout/font/css teams.

:gbrown, do you have a preferred next step?

Comment 15

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #14)
> The ideal situation is to have auto-retry, but that isn't supported with the
> windows worker (although the GPU jobs retry...possibly that is something to
> look into more).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01681f864394d8258ce8a2cbc506908ad3debb6c

I checked into retry a little more. I found that buildbot jobs retry OK, but Windows taskcluster jobs do not (even gpu). I tried adding taskcluster retry support for the generic worker, but could not get it to work (probably needs something in the worker implementation...I don't really know). https://treeherder.mozilla.org/#/jobs?repo=try&revision=01681f864394d8258ce8a2cbc506908ad3debb6c

Comment 16

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #14)
> :gbrown, do you have a preferred next step?

I added some suggestions to bug 1343049.

Comment 17

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

it appears we have solved this in bug 1343049, so lets close this out.