ThemeServiceMaterialDesignTest.SeparatorColor fails on win beta for official.desktop.continuous


Link to Builder:
=================

https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20beta/builds/2309

Link to Error Log:
===================

https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20beta/builds/2309/steps/unit_tests/logs/stdio

The failure line:

c:\b\build\slave\win_beta\build\src\chrome\browser\themes\theme_service_unittest.cc(452): error: Expected: (separator_luminance) < (frame_luminance), actual: 0.462077 vs 0.178661

I don't know what color has a relative luminance of 0.178661; that's very dark compared with what we expect the default frame color (MD or not) to be, and it doesn't match the incognito frame colors either.

I'll probably need to add more logging to the test to dump the different colors on failure, then look for where the actual color might be found.

Update: The above test failure is still seen on latest Win_beta for official.desktop.continuous.

Link to the builder
-------------------
https://chromegw.corp.google.com/i/official.desktop.continuous/builders/win%20beta/builds/2354


Link to the Error log
---------------------
https://chromegw.corp.google.com/i/official.desktop.continuous/builders/win%20beta/builds/2354/steps/unit_tests/logs/stdio


Thank you!

Update: The above test failure is still seen on latest Win_beta & precise64_beta for official.desktop.continuous.

Link to the builder
-------------------
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20beta/builds/2372


Link to the Error log
---------------------
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20beta/builds/2372/steps/unit_tests/logs/stdio

@pkasting: Hey, would you mind providing an update on the above issue?

Thank you!

Please try to resolve this issue ASAP as we're very close to Desktop-M50 Stable candidate cut.

Not a blocker, we do not block any releases for test failures.
But it would be good to have a fix soon.

Yeah, agreed.

@5: Please don't post any more "update: this still fails" posts.  This will continue to fail until I fix.  If I haven't posted, there's no update.

I hope to check in a more verbose diagnostic today, but I won't be fixing until at least next week.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e5c7d66b4201f46f83ec02ba43a82a0936af971

commit 1e5c7d66b4201f46f83ec02ba43a82a0936af971
Author: pkasting <pkasting@chromium.org>
Date: Fri Apr 08 02:59:11 2016

Add more tracing to a test to make it easier to track down failures.

BUG= 599222 
TEST=none

Review URL: https://codereview.chromium.org/1872683002

Cr-Commit-Position: refs/heads/master@{#385954}

[modify] https://crrev.com/1e5c7d66b4201f46f83ec02ba43a82a0936af971/chrome/browser/themes/theme_service_unittest.cc

Sigh.  I need to merge the change from comment 11 to see why this is failing on M-50 branch builders.

So requesting.  Release managers, this only affects a unittest, and only to add more logging, so it should be rubber-stampable.

[Automated comment] Less than 2 weeks to go before stable on M50, manual review required.

Approving merge to M50 branch (2661) based on comment #13. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcaa16d8cec82c01b2af4cd6c27b2fc40706fdb8

commit fcaa16d8cec82c01b2af4cd6c27b2fc40706fdb8
Author: Peter Kasting <pkasting@chromium.org>
Date: Fri Apr 15 21:24:32 2016

Add more tracing to a test to make it easier to track down failures.

BUG= 599222 
TEST=none

Review URL: https://codereview.chromium.org/1872683002

Cr-Commit-Position: refs/heads/master@{#385954}
(cherry picked from commit 1e5c7d66b4201f46f83ec02ba43a82a0936af971)

Review URL: https://codereview.chromium.org/1893733002 .

Cr-Commit-Position: refs/branch-heads/2661@{#588}
Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081}

[modify] https://crrev.com/fcaa16d8cec82c01b2af4cd6c27b2fc40706fdb8/chrome/browser/themes/theme_service_unittest.cc

This fails on 50.0.2661.86, Windows and Precise 64 on Official Desktop continuous builder

Link to the builder:
====================
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20stable/builds/555
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/precise64%20stable/builds/526

Link to the log:
=================
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20stable/builds/555/steps/unit_tests/logs/stdio
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/precise64%20stable/builds/526/steps/unit_tests/logs/stdio

pkasting@: Could you please take a look and confirm if anything actionable to track down from the recent failures.

Thank you!

The issue is that https://codereview.chromium.org/1798323002 is not in M50.

That change is described as affecting MD on Linux/Windows, which means it would be a no-op in M50, since those platforms are non-MD in that release.

However, I'm concerned that the actual change looks as if maybe it affects the non-MD colors?  Or does it not matter because those platforms only ever render with images, and ignore the frame color entirely, pre-MD?

We have three choices here:
* Merge Evan's change
* Disable the test
* Ignore the failure and close this bug

Evan, do you have input on the right fix here?

That unit test explicitly enables MD, which is why this is failing on M50 even though MD is not default there. If that understanding is correct, I think we can just ignore the failure.

"""
However, I'm concerned that the actual change looks as if maybe it affects the non-MD colors?  Or does it not matter because those platforms only ever render with images, and ignore the frame color entirely, pre-MD?
"""

That does seem worrisome, but I think that you're correct the frame color isn't used pre-MD. Instead, BrowserThemePack::CreateFrameImages always generates images.

Could you please provide solution for this?

Thanks

I'm not sure what sort of solution you're looking for?

Given comment 20, I don't think we should merge back Evan's change.  In that case, the test failure here is benign.  There are thus two possible resolutions:

1. Commit a change just to the 2661 branch to disable the test on Windows.
2. Simply ignore the test failure; close this bug and do nothing.

If the release managers don't mind having the test failure in the logs, I'd do option 2.  Otherwise I can do option 1.  It's up to whomever owns the branch.

Ccing Tina [M50 Desktop TPM]

Clearly this didn't block 50, so I'm going to close WontFix.