Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1861793002:
Add more layout tests for DocumentWriteEvaluator (Closed)
|
Created:
4 years, 8 months ago by Charlie Harrison Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd more layout tests for DocumentWriteEvaluator
BUG=599115
Committed: https://crrev.com/5acddabf8ffe48de4508fbdf88066b6f6f4e9c8c
Cr-Commit-Position: refs/heads/master@{#387523}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Bryan initial responses #Patch Set 3 : Changes copied from kinuko@ + fix flaky test #Patch Set 4 : Remove code that's already landed #Patch Set 5 : Added virtual change as dependent patchset #Patch Set 6 : Actually merge in the dependent patchset #
Total comments: 11
Patch Set 7 : jkarlin review #
Total comments: 4
Patch Set 8 : jkarlin@ review #
Messages
Total messages: 33 (10 generated)
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org
PTAL. Let me know if you think of more things to test.
Thanks for these! A couple comments. What causes the doc.write preload scanner to get enabled for these tests? Is there a way to turn it off in some tests? https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload.html (right): https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload.html:6: // We reject scripts with "for" this comment seems out of place https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload_stubs.html (right): https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload_stubs.html:12: var src = window.location.protocol + '//' + window.location.hostname + ':8000' + '/resources/dummy.js'; IIUC cross origin resources report timing values of zero. https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload_stubs.html:19: assert_less_than(r.startTime, boundedStart); given the above, we should probably assert that r.startTime > 0 as well (and in the other tests as well)
csharrison@chromium.org changed reviewers: + kinuko@chromium.org
Adding kinuko@ for LayoutTest help. I tried adding an entry to VirtualTestSuites, but then all tests in http/tests/preload fail due to the testharness not being properly initialized. I think that can be fixed by using the alias "js-test-resources", but many of the tests use other resources not in LayoutTests/resources. What do you recommend? https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload.html (right): https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload.html:6: // We reject scripts with "for" On 2016/04/05 20:34:09, Bryan McQuade wrote: > this comment seems out of place It describes why we changed "window.performance" to "window.perf". I'll update to make it more clear. https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload_stubs.html (right): https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload_stubs.html:12: var src = window.location.protocol + '//' + window.location.hostname + ':8000' + '/resources/dummy.js'; On 2016/04/05 20:34:09, Bryan McQuade wrote: > IIUC cross origin resources report timing values of zero. Does this count as cross origin? I'm seeing valid startTimes locally. https://codereview.chromium.org/1861793002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/preload/document_write_preload_stubs.html:19: assert_less_than(r.startTime, boundedStart); On 2016/04/05 20:34:09, Bryan McQuade wrote: > given the above, we should probably assert that r.startTime > 0 as well (and in > the other tests as well) Done.
> Adding kinuko@ for LayoutTest help. > > I tried adding an entry to VirtualTestSuites, but then all tests in > http/tests/preload fail due to the testharness not being properly initialized. I > think that can be fixed by using the alias "js-test-resources", but many of the > tests use other resources not in LayoutTests/resources. > > What do you recommend? You shouldn't need such trick... also if we use virtual test you shouldn't need to use status=test in RunTimeEnableFeatures. But actually I found that the current test infra doesn't seem to really work for our tests. I've looked into the test code and it looks we need a tiny tweak... I created a separate patch for the part (I'm not fully sure if the tweak is the right fix but something looks wrong): https://codereview.chromium.org/1861313002/ Here's the fixed version of this patch (with the tweak mentioned above): https://codereview.chromium.org/1859383002/, you could probably follow the setting part etc (By the way the tests look a bit flaky even when the flag is enabled?)
Thanks for investigating this! Indeed it makes sense to me now that the virtual tests were not being run as http tests. I can re-take over the CL with your changes and fix the flaky behavior.
Were you seeing flakiness for anything except the "without_doc_write_evaluator"? That one should fail with the experiment flag on (basically testing that the experiment set up is correct).
Sorry, the flakiness was an oversight that I've fixed. I incorporated your changes here. Thanks again for helping out with this.
Fyi, Tools/Scripts/webkitpy change was landed (in a slightly different way), so you can drop manager.py change from your patch set.
On 2016/04/07 at 07:30:38, kinuko wrote: > Fyi, Tools/Scripts/webkitpy change was landed (in a slightly different way), so you can drop manager.py change from your patch set. Thanks. I've updated the patchset. Bryan, can you take another look?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861793002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861793002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
csharrison@chromium.org changed reviewers: + jkarlin@chromium.org
+jkarlin cause Shivani's layout tests need this turned off by default.
Actually, I'm just going to split the layout test change with the REF / virtual change so we can get that in first.
LGTM https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/without_doc_write_evaluator.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/without_doc_write_evaluator.html:20: assert_greater_than(r.startTime, 0); this seems redundant in the case where the above assert is a greater than test. you could consider doing this assert first to make sure we got a non-zero startTime.
https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:5: var t = async_test('Do not preload for document.write for long scripts or scripts that contain non-determinism'); This file needs to be reindented. Code between <script> and </script> should be indented. I also see 4 spaces in places and 2 in others. https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:16: </script> Can you add newlines between <script> sections to make this easier to read? https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:38: if (r.name.indexOf('dummy.js') != -1) { The test doesn't verify that the resources were actually discovered. Perhaps count each found resource and verify that the count is 3 at the end? https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload_stubs.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload_stubs.html:5: var t = async_test('Accessors like location should be properly stubbed to aid in preloading scripts injected via document.write'); This file also needs to be reindented https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/without_doc_write_evaluator.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/without_doc_write_evaluator.html:5: var t = async_test("Make sure that timing assumptions are accurate when document write preloading is off"); This file also needs to be reindented
Thanks! PTAL! https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:5: var t = async_test('Do not preload for document.write for long scripts or scripts that contain non-determinism'); On 2016/04/12 at 13:19:02, jkarlin wrote: > This file needs to be reindented. Code between <script> and </script> should be indented. I also see 4 spaces in places and 2 in others. Done. https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:16: </script> On 2016/04/12 at 13:19:02, jkarlin wrote: > Can you add newlines between <script> sections to make this easier to read? Done. https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:38: if (r.name.indexOf('dummy.js') != -1) { On 2016/04/12 at 13:19:02, jkarlin wrote: > The test doesn't verify that the resources were actually discovered. Perhaps count each found resource and verify that the count is 3 at the end? Good point. Done. https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload_stubs.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_preload_stubs.html:5: var t = async_test('Accessors like location should be properly stubbed to aid in preloading scripts injected via document.write'); On 2016/04/12 at 13:19:02, jkarlin wrote: > This file also needs to be reindented Done. https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/without_doc_write_evaluator.html (right): https://codereview.chromium.org/1861793002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/without_doc_write_evaluator.html:20: assert_greater_than(r.startTime, 0); On 2016/04/11 at 19:52:24, Bryan McQuade wrote: > this seems redundant in the case where the above assert is a greater than test. you could consider doing this assert first to make sure we got a non-zero startTime. Done.
lgtm with comments https://codereview.chromium.org/1861793002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html (right): https://codereview.chromium.org/1861793002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:13: // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX Can you wrap this exceptionally long line? https://codereview.chromium.org/1861793002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:55: assert_greater_than_equal(count, 2); It should be equal right?
The CQ bit was checked by csharrison@chromium.org
Thanks! https://codereview.chromium.org/1861793002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html (right): https://codereview.chromium.org/1861793002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:13: // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX On 2016/04/14 at 17:51:56, jkarlin wrote: > Can you wrap this exceptionally long line? Haha yeah. Done. https://codereview.chromium.org/1861793002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/document-write/document_write_no_preload.html:55: assert_greater_than_equal(count, 2); On 2016/04/14 at 17:51:57, jkarlin wrote: > It should be equal right? The Math.random() makes this non-deterministic. I've updated the test so it always document.writes() a non-deterministic string.
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/1861793002/#ps140001 (title: "jkarlin@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861793002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861793002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861793002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861793002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add more layout tests for DocumentWriteEvaluator BUG=599115 ========== to ========== Add more layout tests for DocumentWriteEvaluator BUG=599115 Committed: https://crrev.com/5acddabf8ffe48de4508fbdf88066b6f6f4e9c8c Cr-Commit-Position: refs/heads/master@{#387523} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5acddabf8ffe48de4508fbdf88066b6f6f4e9c8c Cr-Commit-Position: refs/heads/master@{#387523} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
