Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1687803002:
Expand WebEmbeddedWorkerImplTest (Closed)
DescriptionExpand WebEmbeddedWorkerImplTest
Added more thorough tests.
Committed: https://crrev.com/7d40a0407d4765e92626570ef86dde0542af550a
Cr-Commit-Position: refs/heads/master@{#375190}
Patch Set 1 #Patch Set 2 : git-cl format #Patch Set 3 : presubmit #Patch Set 4 : rebase #
Messages
Total messages: 22 (8 generated)
falken@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, this patch hits the PRESUBMIT warning: "You should be using FrameTestHelpers::pumpPendingRequests() instead of serveAsynchronousMockedRequests()" However, pumpPendingRequestsDoNotUse is also deprecated, and the recommended loadFrame functions don't seem to work here because WebEmbeddedWorkerImpl calls load() by itself, so these tests would wrongly load twice. In addition, using pumpPendingRequestsDoNotUse itself was flaky for some reason. Sometimes the Success test would hang because ServeAsyncRequestsTask gets in a loop by m_client->isLoading() is always true. I'm guessing this is some timing issue because pumpPendingRequestsDoNotUse posts a task whereas serveAsynchronousMockedRequests doesn't? I'm guessing the warnings aren't applicable to loading a worker script, since we don't case about parsing HTML and doing layout. Can you help? For more context see also https://codereview.chromium.org/1675613002/, this patch is broken off from there.
/sub, as I've been feeling the loading test code would need to be cleaned up a bit.. I think for raw resource loadings which don't involve threaded html parser just using serveAsynchronousMockedRequests should be fine-- but please wait for dcheng's reply :)
Description was changed from ========== Expand WebEmbeddedWorkerImplTest Added more thorough tests. ========== to ========== Expand WebEmbeddedWorkerImplTest Added more thorough tests. ==========
On 2016/02/10 at 07:11:20, kinuko wrote: > /sub, as I've been feeling the loading test code would need to be cleaned up a bit.. > > I think for raw resource loadings which don't involve threaded html parser just using serveAsynchronousMockedRequests should be fine-- but please wait for dcheng's reply :) falken@, I saw your G+ post about this =) This has come up in the past for workers: https://codereview.chromium.org/383063008/diff/100001/Source/web/tests/Servic... The primary intention of this presubmit is as kinuko@ says: it's to prevent test flakiness due to tests not actually waiting for the threaded html parser to complete. But there are some contexts where this doesn't apply, and there's no better alternative. So just ignore the presubmit: for the actual test, I defer to kinuko@ for the L-G-T-M. Note: I briefly investigated jyasskin's idea, and I still think it's worth doing: I just don't recall what issues I hit that caused me to shelve it for later.
Thanks for the quick reply! I think ignoring the presubmit means I can't CQ which is a shame, and I guess it would trip everytime someone touches this file. Would you be opposed to me modifying the presubmit to whitelist WebEmbeddedWorkerImplTest?
On 2016/02/10 at 08:17:44, falken wrote: > Thanks for the quick reply! I think ignoring the presubmit means I can't CQ which is a shame, and I guess it would trip everytime someone touches this file. Would you be opposed to me modifying the presubmit to whitelist WebEmbeddedWorkerImplTest? That seems reasonable to me: alternately, just make it a warning instead of an error?
falken@chromium.org changed reviewers: + horo@chromium.org, thakis@chromium.org
dcheng@: Thanks. BTW I still your stamp as web/ OWNER :) Or I'll just TBR you if horo/kinuko approve. thakis@: Can you review PRESUBMIT? (See conversation with dcheng@ above.) horo@ or kinuko@: Can you review WebEmbeddedWorkerImplTest?
falken@chromium.org changed reviewers: + nhiroki@chromium.org
Ah, +nhiroki@ who already looked at WebEmbeddedWorkerImplTest in the parent patch.
lgtm
thakis: Can you review PRESUBMIT? dcheng: Can you stamp for web/ (or I'll TBR as you mentioned deferring above)?
RS LGTM
lgtm (generally not a fan of presubmits with false positives -- maybe the presubmit can be improved later on?)
The CQ bit was checked by falken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1687803002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687803002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687803002/60001
Message was sent while issue was closed.
Description was changed from ========== Expand WebEmbeddedWorkerImplTest Added more thorough tests. ========== to ========== Expand WebEmbeddedWorkerImplTest Added more thorough tests. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Expand WebEmbeddedWorkerImplTest Added more thorough tests. ========== to ========== Expand WebEmbeddedWorkerImplTest Added more thorough tests. Committed: https://crrev.com/7d40a0407d4765e92626570ef86dde0542af550a Cr-Commit-Position: refs/heads/master@{#375190} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7d40a0407d4765e92626570ef86dde0542af550a Cr-Commit-Position: refs/heads/master@{#375190} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
