Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1442593002:
The AllUrlsApiTest.WhitelistedExtension test should no longer flake. (Closed)
|
Created:
5 years, 1 month ago by Alexander Dunaev Modified:
5 years ago Reviewers:
Finnur CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe AllUrlsApiTest.WhitelistedExtension test should no longer flake.
The test had begun to hang after the refactoring that changed the way the extensions are loaded and reloaded. As a result of that refactoring, the test loaded the pages and tried to get some response from the extensions before they had time to reload.
This change introduces waiting for the extensions reload, so the test should no longer flake.
BUG=174341
R=finnur@chromium.org
Committed: https://crrev.com/454083864ede93660c2f08cf7d0e06ca10013c0d
Cr-Commit-Position: refs/heads/master@{#359537}
Patch Set 1 #
Total comments: 2
Patch Set 2 : The review issue is addressed. #Patch Set 3 : The WhitelistedExtensionRunsOnExtensionPages test is enabled. #Messages
Total messages: 28 (9 generated)
Description was changed from ========== The AllUrlsApiTest.WhitelistedExtension test should no longer flake. The test had begun to hang after the refactoring that changed the way the extensions are loaded and reloaded. As a result of that refactoring, the test loaded the pages and tried to get some response from the extensions before they had time to reload. BUG=174341 R=finnur@chromium.org ========== to ========== The AllUrlsApiTest.WhitelistedExtension test should no longer flake. The test had begun to hang after the refactoring that changed the way the extensions are loaded and reloaded. As a result of that refactoring, the test loaded the pages and tried to get some response from the extensions before they had time to reload. This change introduces waiting for the extensions reload, so the test should no longer flake. BUG=174341 R=finnur@chromium.org ==========
Ooh, thanks for fixing the test! :) LGTM, with a couple of nits. https://codereview.chromium.org/1442593002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/all_urls_apitest.cc (right): https://codereview.chromium.org/1442593002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/all_urls_apitest.cc:38: content::BrowserContext* /*browser_context*/, Nit: convert to: content::BrowserContext*, // |browser_context|, Although as a comment this isn't useful. I'd just remove it. https://codereview.chromium.org/1442593002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/all_urls_apitest.cc:161: // TODO(devlin): Why? Can it be enabled now?
On 2015/11/12 15:27:19, Finnur wrote: > Ooh, thanks for fixing the test! :) > > LGTM, with a couple of nits. > > https://codereview.chromium.org/1442593002/diff/1/chrome/browser/extensions/a... > File chrome/browser/extensions/all_urls_apitest.cc (right): > > https://codereview.chromium.org/1442593002/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/all_urls_apitest.cc:38: content::BrowserContext* > /*browser_context*/, > Nit: convert to: > > content::BrowserContext*, // |browser_context|, > > Although as a comment this isn't useful. I'd just remove it. > > https://codereview.chromium.org/1442593002/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/all_urls_apitest.cc:161: // TODO(devlin): Why? > Can it be enabled now? Done!
The CQ bit was checked by voodoo@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org Link to the patchset: https://codereview.chromium.org/1442593002/#ps20001 (title: "The review issue is addressed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442593002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442593002/20001
The CQ bit was unchecked by commit-bot@chromium.org
The author voodoo@yandex-team.ru has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Looks like the WhitelistedExtensionRunsOnExtensionPages test can be enabled as well; doing so in the update.
The CQ bit was checked by voodoo@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org Link to the patchset: https://codereview.chromium.org/1442593002/#ps40001 (title: "The WhitelistedExtensionRunsOnExtensionPages test is enabled.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442593002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442593002/40001
The CQ bit was unchecked by commit-bot@chromium.org
The author voodoo@yandex-team.ru has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
I suspect that after you sign the license agreement you can retry sending the patch to the CQ.
The CQ bit was checked by voodoo@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442593002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442593002/40001
Yes indeed. That's fixed now.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/454083864ede93660c2f08cf7d0e06ca10013c0d Cr-Commit-Position: refs/heads/master@{#359537}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1469813003/ by magjed@chromium.org. The reason for reverting is: The WhitelistedExtension is still flaky on the XP Tests bot, see e.g. https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build... stdio output: [ RUN ] AllUrlsApiTest.WhitelistedExtensionRunsOnExtensionPages [1636:4084:1120/124530:WARNING:raw_channel.cc(208)] Shutting down RawChannel with write buffer nonempty [139/464] AllUrlsApiTest.WhitelistedExtensionRunsOnExtensionPages (TIMED OUT).
Message was sent while issue was closed.
Description was changed from ========== The AllUrlsApiTest.WhitelistedExtension test should no longer flake. The test had begun to hang after the refactoring that changed the way the extensions are loaded and reloaded. As a result of that refactoring, the test loaded the pages and tried to get some response from the extensions before they had time to reload. This change introduces waiting for the extensions reload, so the test should no longer flake. BUG=174341 R=finnur@chromium.org Committed: https://crrev.com/454083864ede93660c2f08cf7d0e06ca10013c0d Cr-Commit-Position: refs/heads/master@{#359537} ========== to ========== The AllUrlsApiTest.WhitelistedExtension test should no longer flake. The test had begun to hang after the refactoring that changed the way the extensions are loaded and reloaded. As a result of that refactoring, the test loaded the pages and tried to get some response from the extensions before they had time to reload. This change introduces waiting for the extensions reload, so the test should no longer flake. BUG=174341 R=finnur@chromium.org Committed: https://crrev.com/454083864ede93660c2f08cf7d0e06ca10013c0d Cr-Commit-Position: refs/heads/master@{#359537} ==========
Message was sent while issue was closed.
That's a bit of a drastic action for a test failure on a platform we're rapidly loosing interest in... Wouldn't it have been more prudent to just disable the test again?
Message was sent while issue was closed.
On 2015/11/23 11:47:34, Finnur wrote: > That's a bit of a drastic action for a test failure on a platform we're rapidly > loosing interest in... > > Wouldn't it have been more prudent to just disable the test again? I'm sheriff and don't have time to look into each CL carefully. This CL looks like the cause of failing bot. Please reland when it does not cause any failing bots. The XP bot is still one of the bots we have to guard.
Message was sent while issue was closed.
We could reland it with the updated condition that disables the test on Windows only.
Message was sent while issue was closed.
Yup. That seems like the right approach. Make sure to comment it in such a way that it is obvious it fails on Windows XP. |
