Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781623008:
More thorough overlapping cursor tests. (Closed)
DescriptionMore thorough overlapping cursor tests.
BUG=705837
Review-Url: https://codereview.chromium.org/2781623008
Cr-Commit-Position: refs/heads/master@{#461932}
Committed: https://chromium.googlesource.com/chromium/src/+/0c187ee3c2b8fc9d0dd81758f892cf6fd0ba4c3f
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addressed feedback. #
Messages
Total messages: 27 (19 generated)
The CQ bit was checked by pwnall@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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by pwnall@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.
pwnall@chromium.org changed reviewers: + dmurph@chromium.org, jsbell@chromium.org
jsbell: PTAL? dmurph: You had concerns that the original test was too complex, before I made the code structure more explicit. Can you please take a look to make sure this CL isn't a regression in that aspect? None of these modifications uncovered the clusterfuzz crash. I think the tests are still valuable because they cover new access patterns. Also, the original test has been slightly extended to cover the case where the iterators reach their end (request.result is null after cursor.continue() succeeds).
lgtm with documentation nit https://codereview.chromium.org/2781623008/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interleaved-overlapping-cursors.html (right): https://codereview.chromium.org/2781623008/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interleaved-overlapping-cursors.html:4: <title>IndexedDB: Interleaved iteration of multiple overlapping cursors</title> How is this one different from the the one above? What does an overlapping cursor mean? IT looks like we have 1 cursor for the populate, but then we use the param for reading? Although maybe this is just to store keys and values... Just looking for a quick sentence that differentiates from the one above. Probably one sentence per test - if you can make it super small then just incorporate it in the test string.
lgtm with same nit as dmurph https://codereview.chromium.org/2781623008/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interleaved-overlapping-cursors.html (right): https://codereview.chromium.org/2781623008/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interleaved-overlapping-cursors.html:4: <title>IndexedDB: Interleaved iteration of multiple overlapping cursors</title> On 2017/03/29 23:35:20, dmurph wrote: > How is this one different from the the one above? What does an overlapping > cursor mean? > > IT looks like we have 1 cursor for the populate, but then we use the param for > reading? Although maybe this is just to store keys and values... > > Just looking for a quick sentence that differentiates from the one above. > Probably one sentence per test - if you can make it super small then just > incorporate it in the test string. +1
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Thank you very much for the feedback! https://codereview.chromium.org/2781623008/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interleaved-overlapping-cursors.html (right): https://codereview.chromium.org/2781623008/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interleaved-overlapping-cursors.html:4: <title>IndexedDB: Interleaved iteration of multiple overlapping cursors</title> On 2017/03/29 23:35:20, dmurph wrote: > How is this one different from the the one above? What does an overlapping > cursor mean? > > IT looks like we have 1 cursor for the populate, but then we use the param for > reading? Although maybe this is just to store keys and values... > > Just looking for a quick sentence that differentiates from the one above. > Probably one sentence per test - if you can make it super small then just > incorporate it in the test string. Done. Thanks for the suggestion! I bet it will be helpful when looking at this test years later :D
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.
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, dmurph@chromium.org Link to the patchset: https://codereview.chromium.org/2781623008/#ps60001 (title: "Addressed feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491355110591740,
"parent_rev": "0f7f2aba1845c834c9ce19b76abeff059b6f8f2e", "commit_rev":
"0c187ee3c2b8fc9d0dd81758f892cf6fd0ba4c3f"}
Message was sent while issue was closed.
Description was changed from ========== More thorough overlapping cursor tests. BUG=705837 ========== to ========== More thorough overlapping cursor tests. BUG=705837 Review-Url: https://codereview.chromium.org/2781623008 Cr-Commit-Position: refs/heads/master@{#461932} Committed: https://chromium.googlesource.com/chromium/src/+/0c187ee3c2b8fc9d0dd81758f892... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0c187ee3c2b8fc9d0dd81758f892...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.chromium.org/2803563002/ by maxmorin@chromium.org. The reason for reverting is: external/wpt/IndexedDB/parallel-overlapping-cursors.html is failing on the win7 webkit bot: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%....
Message was sent while issue was closed.
looks like a timeout, taking longer than 90 seconds. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
