Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570553002:
Clear cache before running the test (Closed)
|
Created:
4 years ago by Sergiy Byelozyorov Modified:
3 years, 11 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear cache before running the test
This should help reducing the flakiness of this test, which is currently ~65%:
http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. The
text diff shows that actual number of items that are cached is different from
the expected: https://justpaste.it/11bq3.
R=allada@chromium.org
BUG=673378
Patch Set 1 #Patch Set 2 : Fix #
Total comments: 2
Patch Set 3 : Addressed comments #Patch Set 4 : Addressed comments #Patch Set 5 : Fix #Patch Set 6 : Clear cache synchronously #Patch Set 7 : Fix #
Total comments: 2
Patch Set 8 : Addressed comments #Patch Set 9 : Fix #
Total comments: 4
Patch Set 10 : Addressed comments #
Messages
Total messages: 49 (28 generated)
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Clear cache before running the test R=tandrii@chromium.org ========== to ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65.88%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. R=allada@chromium.org ==========
sergiyb@chromium.org changed reviewers: + allada@chromium.org, pfeldman@chromium.org - tandrii@chromium.org
Description was changed from ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65.88%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. R=allada@chromium.org ========== to ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65.88%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. R=allada@chromium.org BUG=673378 ==========
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65.88%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. R=allada@chromium.org BUG=673378 ========== to ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. The text diff shows that actual number of items that are cached is different from the expected. R=allada@chromium.org BUG=673378 ==========
Description was changed from ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. The text diff shows that actual number of items that are cached is different from the expected. R=allada@chromium.org BUG=673378 ========== to ========== Clear cache before running the test This should help reducing the flakiness of this test, which is currently ~65%: http://test-results.appspot.com/flakiness/dir/http/tests/inspector/network. The text diff shows that actual number of items that are cached is different from the expected: https://justpaste.it/11bq3. R=allada@chromium.org BUG=673378 ==========
lgtm https://codereview.chromium.org/2570553002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html (right): https://codereview.chromium.org/2570553002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html:101: function clearAllCaches() { I would love to have this moved into the network-test.js file.
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
https://codereview.chromium.org/2570553002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html (right): https://codereview.chromium.org/2570553002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html:101: function clearAllCaches() { On 2016/12/12 17:45:25, Blaise wrote: > I would love to have this moved into the network-test.js file. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
There already is http/tests/inspector/network-test.js, I guess Blaise meant that one.
On 2016/12/12 20:45:06, pfeldman wrote: > There already is http/tests/inspector/network-test.js, I guess Blaise meant that > one. Ah, I see. Thanks for the pointer, I wasn't looking for it there... thought it's similar to cache-storage-test.js, which is located inside cache-storage folder.
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
https://codereview.chromium.org/2570553002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html (right): https://codereview.chromium.org/2570553002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html:88: InspectorTest.clearAllCaches(); Why do you think Cache API is related to the flakiness?
Most of our network tests should be ran with cache either disabled or cleared. Right now I believe we do not have a callback to our "ClearBrowserCache" protocol command, so we cannot know when it finished clearing. (see: https://cs.chromium.org/chromium/src/chrome/browser/browsing_data/browsing_da...) The way I hacked around this in other tests is by calling: InspectorTest.NetworkAgent.setCacheDisabled(true, InspectorTest.NetworkAgent.setCacheDisabled.bind(InspectorTest.NetworkAgent, finishedCallback)); This call should clear cache then turn it back on. I would suggest wrapping this in a function in the network-test.js file. I think we actually want to run this call most of the time when InspectorTest.recordNetwork is called. So, if you would like, I think we could fix most of our newly created flakeyness tests (due to a recycle of memory cache between page loads that happened earlier this year) by changing the recordNetwork() call to have a parameter of "preserveCache" and by default clear the cache. There shouldn't be any tests that this will cause a problem with, but if there is they should fail consistently. https://codereview.chromium.org/2570553002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js (right): https://codereview.chromium.org/2570553002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:59: .then((keys) => Promise.all(keys.map((key) => caches.delete(key)))) I see you pulled this from http/tests/inspector/cache-storage/cache-storage-test.js, however I believe this is for the cache storage api not the browser cache.
Sorry, I meant: InspectorTest.NetworkAgent.setCacheDisabled(true, InspectorTest.NetworkAgent.setCacheDisabled.bind(InspectorTest.NetworkAgent, false, finishedCallback));
> Why do you think Cache API is related to the flakiness? Sorry, I did not realize that Cache API is unrelated to browser cache. > InspectorTest.NetworkAgent.setCacheDisabled(true, > InspectorTest.NetworkAgent.setCacheDisabled.bind(InspectorTest.NetworkAgent, > false, finishedCallback)); Thanks, rewrote CL using your suggestion.
Looks good! https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js (right): https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:126: InspectorTest.NetworkAgent.setCacheDisabled(true, function() { Our new code does not use anonymous functions, we use either bind on named functions or small arrow functions. I'd suggest: var networkAgent = InspectorTest.NetworkAgent; networkAgent.setCacheDisabled(true, networkAgent.setCacheDisabled.bind(networkAgent, false, finishedCallback)); https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html (right): https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html:89: function fetchResources() { nit: This is very nitty, but could we move this above? I like to make my tests call order of function not jump around as much as possible, especially for tests.
https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js (right): https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector/network-test.js:126: InspectorTest.NetworkAgent.setCacheDisabled(true, function() { On 2016/12/16 22:47:28, allada wrote: > Our new code does not use anonymous functions, we use either bind on named > functions or small arrow functions. > > I'd suggest: > > var networkAgent = InspectorTest.NetworkAgent; > networkAgent.setCacheDisabled(true, > networkAgent.setCacheDisabled.bind(networkAgent, false, finishedCallback)); Done. https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html (right): https://codereview.chromium.org/2570553002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector/network/network-filters.html:89: function fetchResources() { On 2016/12/16 22:47:28, allada wrote: > nit: This is very nitty, but could we move this above? I like to make my tests > call order of function not jump around as much as possible, especially for > tests. Done.
The CQ bit was checked by sergiyb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org Link to the patchset: https://codereview.chromium.org/2570553002/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Hi, I've just added network-filters.html to TestExpectations. Can you removes the entry if this CL fixes the flakiness? Thanks.
Message was sent while issue was closed.
On 2017/01/06 06:21:06, yhirano wrote: > Hi, I've just added network-filters.html to TestExpectations. Can you removes > the entry if this CL fixes the flakiness? Thanks. Thanks. In fact your change is doing just what I wanted. I'll now be able to verify if the test flakiness drops on the Flakiness Surface app. Since this change is taking way longer than it should, I am going to close it. If someone wants to continue and fix this, feel free to apply patch locally and re-upload.
Message was sent while issue was closed.
On 2017/01/09 14:33:37, SergiyB (OOO till Jan 8) wrote: > On 2017/01/06 06:21:06, yhirano wrote: > > Hi, I've just added network-filters.html to TestExpectations. Can you removes > > the entry if this CL fixes the flakiness? Thanks. > > Thanks. In fact your change is doing just what I wanted. I'll now be able to > verify if the test flakiness drops on the Flakiness Surface app. Since this > change is taking way longer than it should, I am going to close it. If someone > wants to continue and fix this, feel free to apply patch locally and re-upload. No need to close it. It's done. You should be able to commit it now.
On 2017/01/09 16:42:00, allada wrote: > On 2017/01/09 14:33:37, SergiyB (OOO till Jan 8) wrote: > > On 2017/01/06 06:21:06, yhirano wrote: > > > Hi, I've just added network-filters.html to TestExpectations. Can you > removes > > > the entry if this CL fixes the flakiness? Thanks. > > > > Thanks. In fact your change is doing just what I wanted. I'll now be able to > > verify if the test flakiness drops on the Flakiness Surface app. Since this > > change is taking way longer than it should, I am going to close it. If someone > > wants to continue and fix this, feel free to apply patch locally and > re-upload. > > No need to close it. It's done. You should be able to commit it now. I'll try to CQ this once more, but IIRC last year it failed with some errors that I was not able to decrypt and I don't have the time to investigate in more detail. The change by Yutaka was sufficient for me to check the correctness of the dashboard and now I need to focus on fixing it.
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/09 16:47:59, SergiyB (OOO till Jan 8) wrote: > > I'll try to CQ this once more, but IIRC last year it failed with some errors > that I was not able to decrypt and I don't have the time to investigate in more > detail. The change by Yutaka was sufficient for me to check the correctness of > the dashboard and now I need to focus on fixing it. Yes those errors are super common for us and rarely our issue, we usually just retry them until they pass :-(
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
On 2017/01/09 17:08:26, allada wrote: > On 2017/01/09 16:47:59, SergiyB (OOO till Jan 8) wrote: > > > > I'll try to CQ this once more, but IIRC last year it failed with some errors > > that I was not able to decrypt and I don't have the time to investigate in > more > > detail. The change by Yutaka was sufficient for me to check the correctness of > > the dashboard and now I need to focus on fixing it. > > Yes those errors are super common for us and rarely our issue, we usually just > retry them until they pass :-( Looks like the test times out and I am not sure why. It does not on my machine. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
