Issue metadata
Sign in to add a comment
|
ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange is flaky |
||||||||||||||||||||||||
Issue descriptionFlaky test: ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange Sample failed build due to flakiness: https://ci.chromium.org/buildbot/chromium.mac/Mac10.12%20Tests/16631 Test output log: https://chromium-swarm.appspot.com/task?id=411647bd264b2a10 Culprit (70.0% confidence): r607022 Analysis: https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy_gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCLHAWNocm9taXVtLm1hYy9NYWMxMC4xMiBUZXN0cy8xNjYzMS9uZXR3b3JrX3NlcnZpY2VfYnJvd3Nlcl90ZXN0cyBvbiBJbnRlbCBHUFUgb24gTWFjIG9uIE1hYy0xMC4xMi42L1EyaHliMjFsUW5KdmQzTmxjazFoYVc1Q2NtOTNjMlZ5VkdWemRDNVdZWEpwWVhScGIyNXpVMlZ5ZG1salpWTjBZWEowYzFKbGNYVmxjM1JQYms1bGRIZHZjbXREYUdGdVoyVT0MCxITTWFzdGVyRmxha2VBbmFseXNpcxgBDA Please revert the culprit, or disable the test and find the appropriate owner. If the culprit above is wrong, please file a bug using this link: https://bugs.chromium.org/p/chromium/issues/entry?status=Unconfirmed&labels=Pri-1,Test-Findit-Wrong&components=Tools%3ETest%3EFindit%3EFlakiness&summary=%5BFindit%5D%20Flake%20Analyzer%20-%20Wrong%20result%20for%20ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange&comment=Link%20to%20Analysis%3A%20https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy_gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCLHAWNocm9taXVtLm1hYy9NYWMxMC4xMiBUZXN0cy8xNjYzMS9uZXR3b3JrX3NlcnZpY2VfYnJvd3Nlcl90ZXN0cyBvbiBJbnRlbCBHUFUgb24gTWFjIG9uIE1hYy0xMC4xMi42L1EyaHliMjFsUW5KdmQzTmxjazFoYVc1Q2NtOTNjMlZ5VkdWemRDNVdZWEpwWVhScGIyNXpVMlZ5ZG1salpWTjBZWEowYzFKbGNYVmxjM1JQYms1bGRIZHZjbXREYUdGdVoyVT0MCxITTWFzdGVyRmxha2VBbmFseXNpcxgBDA Automatically posted by the findit-for-me app (https://goo.gl/Ot9f7N).
,
Nov 13
Assigning to author of likely culprit CL.
,
Nov 13
I reverted the CL for now (crrev.com/c/1334421), the starting number of variations seed requests on the failing run is 0, and the ending one is 2, so it seems the retry logic is triggering at some point. (So the bug that the test checks for didn't regress).
,
Nov 13
It seems the browser test goes through the usual path for retrieving the seed so if the first attempt fails it will always cause a retry (but it was not being surfaced prior to this CL since the tests use an HTTP url, so the logic didn't trigger), a simple solution would be to check for one or two requests happening, since the test would still validate a request happened on network changes, but won't fail if a retry happens. Another alternative would be to check both the URL, and whether or not the previous attempt was a retry when retrying, but I'm not too keen about this since it feels like tweaking Chrome's behavior to match a quirk in the test. Variations folks, do you have any opinions on this one? Otherwise I'll write a CL adding the check for one or two requests, and revert the revert.
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/871a1ef511b48acdad53f7d8e8fafd6362fc52d2 commit 871a1ef511b48acdad53f7d8e8fafd6362fc52d2 Author: Carlos IL <carlosil@chromium.org> Date: Tue Nov 27 17:05:29 2018 Reland "Variations service now checks explicitely if previous attempt was HTTP." This reverts commit 8d41f8c9c5f71f94133e1e557701521b5e464c63. Reason for revert: This CL reverts the revert, and fixes the flakiness in the test by allowing one or two requests in the test. Bug: 904108 , 902727 Original change's description: > Revert "Variations service now checks explicitely if previous attempt was HTTP." > > This reverts commit 91d246885ead6155c131e027d99be1aa79b9ae3c. > > Reason for revert: This CL made ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange flaky ( crbug.com/904108 ) reverting while investigating why. > > Original change's description: > > Variations service now checks explicitely if previous attempt was HTTP. > > > > Variations service now sets a flag if the last request was an HTTP > > retry, and uses it to decide whether to retry again, instead of > > relying on the scheme of the final URL. Also, retries are now disabled > > if the fallback url is HTTPS. > > > > Bug: 902727 > > Change-Id: Ibd6df4a22bc6302b231aff0ae32f8e1c8a1f277c > > Reviewed-on: https://chromium-review.googlesource.com/c/1325035 > > Commit-Queue: Carlos IL <carlosil@chromium.org> > > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#607022} > > TBR=asvitkine@chromium.org,carlosil@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 902727 > Change-Id: I67036ea20097f889e71de5179a2e4510a201388d > Reviewed-on: https://chromium-review.googlesource.com/c/1334421 > Reviewed-by: Carlos IL <carlosil@chromium.org> > Commit-Queue: Carlos IL <carlosil@chromium.org> > Cr-Commit-Position: refs/heads/master@{#607882} Change-Id: I5618dd7c48849ddf1fc8cd8f613ba7d83ebc66c0 Reviewed-on: https://chromium-review.googlesource.com/c/1335666 Commit-Queue: Carlos IL <carlosil@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#611152} [modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/chrome/browser/chrome_browser_main_browsertest.cc [modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/components/variations/service/variations_service.cc [modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/components/variations/service/variations_service.h [modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/components/variations/service/variations_service_unittest.cc
,
Nov 27
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by dtapu...@chromium.org
, Nov 12