Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863413005:
Use a virtual test suite for DocumentWriteEvaluator layout tests (Closed)
|
Created:
4 years, 8 months ago by Charlie Harrison Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse a virtual test suite for DocumentWriteEvaluator layout tests
BUG=599115
Committed: https://crrev.com/485e699f2f60b8ccef36f05c040d3d7036be8f1a
Cr-Commit-Position: refs/heads/master@{#386179}
Patch Set 1 #Patch Set 2 : rebase on #386075 #
Total comments: 8
Patch Set 3 : Expect just failure for non virtual doc write tests #
Total comments: 5
Messages
Total messages: 21 (3 generated)
csharrison@chromium.org changed reviewers: + jkarlin@chromium.org, pdr@chromium.org
PTAL! This is a split of https://codereview.chromium.org/1861793002 +pdr for RuntimeEnabledFeatureFeatures.in +jkarlin for LayoutTests/
https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Pass Failure ] Why this change? Weren't the tests working before this CL? https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:26: Why did this script change in this CL?
https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:26: On 2016/04/08 15:17:52, jkarlin wrote: > Why did this script change in this CL? What I mean is, why are there more lines in this file than the deleted one?
https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Pass Failure ] On 2016/04/08 at 15:17:52, jkarlin wrote: > Why this change? Weren't the tests working before this CL? This is because now we're changing the RuntimeEnabledFeatures list. The virtual ones will be virtual/documentwriteevaluator/http/tests/preload/document-write. https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:26: On 2016/04/08 at 15:17:52, jkarlin wrote: > Why did this script change in this CL? The directory changed due to make enable virtual testing just this directory. The code changed slightly to make the test less fragile (i.e. by not assuming the index in the resource entries will always be the same). I'm happy to use the same file though for consistency.
https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Pass Failure ] On 2016/04/08 15:25:12, csharrison wrote: > On 2016/04/08 at 15:17:52, jkarlin wrote: > > Why this change? Weren't the tests working before this CL? > > This is because now we're changing the RuntimeEnabledFeatures list. The virtual > ones will be virtual/documentwriteevaluator/http/tests/preload/document-write. Did you mean to s/document-write/document_write_preload.html/ ? I don't see anything matching what you have.
https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Pass Failure ] On 2016/04/08 15:40:43, jkarlin wrote: > On 2016/04/08 15:25:12, csharrison wrote: > > On 2016/04/08 at 15:17:52, jkarlin wrote: > > > Why this change? Weren't the tests working before this CL? > > > > This is because now we're changing the RuntimeEnabledFeatures list. The > virtual > > ones will be virtual/documentwriteevaluator/http/tests/preload/document-write. > > Did you mean to s/document-write/document_write_preload.html/ ? I don't see > anything matching what you have. I see, the virtual test becomes a separate test case. Ignore my confusion. This should just be [ Failure ] then.
Thanks! https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Pass Failure ] On 2016/04/08 at 15:45:53, jkarlin wrote: > On 2016/04/08 15:40:43, jkarlin wrote: > > On 2016/04/08 15:25:12, csharrison wrote: > > > On 2016/04/08 at 15:17:52, jkarlin wrote: > > > > Why this change? Weren't the tests working before this CL? > > > > > > This is because now we're changing the RuntimeEnabledFeatures list. The > > virtual > > > ones will be virtual/documentwriteevaluator/http/tests/preload/document-write. > > > > Did you mean to s/document-write/document_write_preload.html/ ? I don't see > > anything matching what you have. > > I see, the virtual test becomes a separate test case. Ignore my confusion. This should just be [ Failure ] then. Great point. Done.
https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:19: assert_less_than(r.startTime, boundedStart); Can this test flake? What guarantee is there that the preparser will have started on this before boundedStart is set?
https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:19: assert_less_than(r.startTime, boundedStart); On 2016/04/08 at 16:27:43, jkarlin wrote: > Can this test flake? What guarantee is there that the preparser will have started on this before boundedStart is set? I guess not a hard guarantee, but the html parser will have to fully fetch and evaluate the testharness.js files before the background parser does the scan. I could always add a delay script if we want to be sure.
lgtm with nit https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:19: assert_less_than(r.startTime, boundedStart); On 2016/04/08 16:33:34, csharrison wrote: > On 2016/04/08 at 16:27:43, jkarlin wrote: > > Can this test flake? What guarantee is there that the preparser will have > started on this before boundedStart is set? > > I guess not a hard guarantee, but the html parser will have to fully fetch and > evaluate the testharness.js files before the background parser does the scan. > > I could always add a delay script if we want to be sure. Seems fine as is until troubles arise, the flakiness dashboard looks clean. You might want to add a comment that this could flake though.
On 2016/04/08 at 16:45:42, jkarlin wrote: > lgtm with nit > > https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html (right): > > https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload.html:19: assert_less_than(r.startTime, boundedStart); > On 2016/04/08 16:33:34, csharrison wrote: > > On 2016/04/08 at 16:27:43, jkarlin wrote: > > > Can this test flake? What guarantee is there that the preparser will have > > started on this before boundedStart is set? > > > > I guess not a hard guarantee, but the html parser will have to fully fetch and > > evaluate the testharness.js files before the background parser does the scan. > > > > I could always add a delay script if we want to be sure. > > Seems fine as is until troubles arise, the flakiness dashboard looks clean. You might want to add a comment that this could flake though. Thanks. I'll just update the next dependent CL to include a short delay script. Better safe than sorry. pdr@, ptal for REF changes.
This looks good to me as well, just one nit. https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Failure ] Is there any reason to run these? I think we should mark the non-virtual directory as Skip instead of Failure
https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Failure ] On 2016/04/08 at 17:33:23, pdr wrote: > Is there any reason to run these? I think we should mark the non-virtual directory as Skip instead of Failure I like Failure here because it asserts that the tests are written correctly, and won't pass trivially if the feature is turned off.
On 2016/04/08 at 17:35:33, csharrison wrote: > https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/1863413005/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/TestExpectations:1324: crbug.com/599115 http/tests/preload/document-write [ Failure ] > On 2016/04/08 at 17:33:23, pdr wrote: > > Is there any reason to run these? I think we should mark the non-virtual directory as Skip instead of Failure > > I like Failure here because it asserts that the tests are written correctly, and won't pass trivially if the feature is turned off. OK LGTM
The CQ bit was checked by csharrison@chromium.org
Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863413005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863413005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use a virtual test suite for DocumentWriteEvaluator layout tests BUG=599115 ========== to ========== Use a virtual test suite for DocumentWriteEvaluator layout tests BUG=599115 Committed: https://crrev.com/485e699f2f60b8ccef36f05c040d3d7036be8f1a Cr-Commit-Position: refs/heads/master@{#386179} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/485e699f2f60b8ccef36f05c040d3d7036be8f1a Cr-Commit-Position: refs/heads/master@{#386179} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
