Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2756393002:
Deflake ImageWriterUtilityClient browsertest and clients (Closed)
|
Created:
3 years, 9 months ago by Noel Gordon Modified:
3 years, 9 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeflake ImageWriterUtilityClient browsertest and clients
The image writer browser tests WriteCancel and VerifyCancel added on
crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717.
Re-enable these browser tests. Move all work to the FILE thread both
in the callers (operation.cc, operation_nonchromeos.cc) of the image
writer interface, and in the browser test (to match caller behavior)
to see if this removes the flake on the Windows DEBUG bots (analysis
per crbug.com/690717#c2 suggests that it should).
The browsertest can't test the low-level USB driver code on any port
since our bots don't have USB sticks. So I manually tested using the
the ChromeOS Recovery Tool [1], and I was able to write and verify a
ChromeOS image to my USB stick using this patch on Windows 10.
Covered by existing tests:
image_writer_utility_client_browsertest, image_writer_unittest
[1] http://bit.ly/2jFtsZZ
BUG=690717, 352442
Review-Url: https://codereview.chromium.org/2756393002
Cr-Commit-Position: refs/heads/master@{#458965}
Committed: https://chromium.googlesource.com/chromium/src/+/54af805eac68ac00ef7a6a9f3ee40fb49894792f
Patch Set 1 #Patch Set 2 : Remove FILE check in constructor, add large Cancel delay to see if we can flake the try bots. #Patch Set 3 : Remove Cancel delay used for testing for try flakes. #
Total comments: 2
Patch Set 4 : Add comments about Cancel API. #
Total comments: 2
Patch Set 5 : Remove progress case from Cancel test. #
Messages
Total messages: 60 (43 generated)
The CQ bit was checked by noel@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by noel@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.
The CQ bit was checked by noel@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.
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and client BUG=690717 ========== to ========== Deflake ImageWriterUtilityClient browsertest and client The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. [1] http://bit.ly/2jFtsZZ BUG=690717 ==========
noel@chromium.org changed reviewers: + sammc@google.com, tibell@chromium.org
noel@chromium.org changed reviewers: + sammc@chromium.org - sammc@google.com
PTAL.
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and client The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. [1] http://bit.ly/2jFtsZZ BUG=690717 ========== to ========== Deflake ImageWriterUtilityClient browsertest and client The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717 ==========
lgtm https://codereview.chromium.org/2756393002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc (right): https://codereview.chromium.org/2756393002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:105: void ImageWriterUtilityClient::Cancel(const CancelCallback& cancel_callback) { Is Cancel even used in production code?
lgtm
https://codereview.chromium.org/2756393002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc (right): https://codereview.chromium.org/2756393002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:105: void ImageWriterUtilityClient::Cancel(const CancelCallback& cancel_callback) { Best I can tell, no. Filed crbug.com/703514 about it.
The CQ bit was checked by noel@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...
noel@chromium.org changed reviewers: + jochen@chromium.org
+jochen for OWNERS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and client The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717 ========== to ========== Deflake ImageWriterUtilityClient browsertest and client The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ==========
please ask somebody from the extension teams to review, as they own this directory
noel@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin for extensions.
Can you expand a bit on why you suspect moving work to the file thread will deflake the test? https://codereview.chromium.org/2756393002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc (right): https://codereview.chromium.org/2756393002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:113: if (progress_ || !cancel_) A comment explaining the purpose here would be useful - in particular, what does [!]progress_ indicate?
The CQ bit was checked by noel@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_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2756393002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc (right): https://codereview.chromium.org/2756393002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:113: if (progress_ || !cancel_) On 2017/03/21 15:15:15, Devlin wrote: > A comment explaining the purpose here would be useful - in particular, what does > [!]progress_ indicate? This is a detail about how the image_writer in the utility process works (and a detail I should remove from this browser test). The image_writer in the utility process sends progress, being the number of bytes it has processed for the current operation. The detail is that it always sends a progress 0 when an operation begins). The test doesn't and shouldn't care about this detail. Removed.
On 2017/03/21 15:15:15, Devlin wrote: > Can you expand a bit on why you suspect moving work to the file thread will > deflake the test? ^^ :)
On 2017/03/21 23:52:02, Devlin wrote: > On 2017/03/21 15:15:15, Devlin wrote: > > Can you expand a bit on why you suspect moving work to the file thread will > > deflake the test? > > ^^ :) Was responding to you in another review. Short story is: Built a DEBUG chrome on WIN10 - was not able to reproduce bug 690717 reliably. Changed the Cancel() code @line 111 - task_runner_->PostDelayedTask(FROM_HERE, cancel_callback, kDelay); - used a kDelay >= 600ms - reproduced bug 690717 reliably in DEBUG Win10. Problem seems to do with the task_runner_->PostTask stuff. Why are the callers and this code using a task_runner to bounce requests FILE <-> IO thread? - seems that is a hang over from an early time when this code used UtilityProcessHostClient to control the utility process (needed IO thread for that). What if we moved all work onto the FILE thread, since callers were using that, and got rid of the task_runner_->PostTask? - easy to test locally: make that change. - retain the kDelay >= 600ms - re-run the browser test with those changes - can no longer reproduce bug 690717 in DEBUG Win10.
The CQ bit was checked by noel@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...
On 2017/03/22 01:58:54, noel gordon wrote: > On 2017/03/21 23:52:02, Devlin wrote: > > On 2017/03/21 15:15:15, Devlin wrote: > > > Can you expand a bit on why you suspect moving work to the file thread will > > > deflake the test? > > > > ^^ :) > > Was responding to you in another review. Short story is: Wrote the longer story into http://crbug.com/690717#c2
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)
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and client The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ========== to ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ==========
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots. The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ========== to ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis in crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ==========
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis in crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ========== to ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis per crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ==========
The CQ bit was checked by noel@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.
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis per crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this change on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ========== to ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis per crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this patch on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ==========
On 2017/03/22 03:38:05, noel gordon wrote: > On 2017/03/22 01:58:54, noel gordon wrote: > > On 2017/03/21 23:52:02, Devlin wrote: > > > On 2017/03/21 15:15:15, Devlin wrote: > > > > Can you expand a bit on why you suspect moving work to the file thread > will > > > > deflake the test? > > > > > > ^^ :) > > > > Was responding to you in another review. Short story is: > > Wrote the longer story into http://crbug.com/690717#c2 lgtm; thanks for the explanation!
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, tibell@chromium.org Link to the patchset: https://codereview.chromium.org/2756393002/#ps80001 (title: "Remove progress case from Cancel test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/22 23:35:46, Devlin wrote: > On 2017/03/22 03:38:05, noel gordon wrote: > > On 2017/03/22 01:58:54, noel gordon wrote: > > > On 2017/03/21 23:52:02, Devlin wrote: > > > > On 2017/03/21 15:15:15, Devlin wrote: > > > > > Can you expand a bit on why you suspect moving work to the file thread > > will > > > > > deflake the test? > > > > > > > > ^^ :) > > > > > > Was responding to you in another review. Short story is: > > > > Wrote the longer story into http://crbug.com/690717#c2 > > lgtm; thanks for the explanation! No worries. Submitting.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490226385045400,
"parent_rev": "ab18f0e1007508f00e41793c22d9ea3ebdb8a4c3", "commit_rev":
"54af805eac68ac00ef7a6a9f3ee40fb49894792f"}
Message was sent while issue was closed.
Description was changed from ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis per crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this patch on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 ========== to ========== Deflake ImageWriterUtilityClient browsertest and clients The image writer browser tests WriteCancel and VerifyCancel added on crrev.com/449296 flaked on the Windows DEBUG bots, issue 690717. Re-enable these browser tests. Move all work to the FILE thread both in the callers (operation.cc, operation_nonchromeos.cc) of the image writer interface, and in the browser test (to match caller behavior) to see if this removes the flake on the Windows DEBUG bots (analysis per crbug.com/690717#c2 suggests that it should). The browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. So I manually tested using the the ChromeOS Recovery Tool [1], and I was able to write and verify a ChromeOS image to my USB stick using this patch on Windows 10. Covered by existing tests: image_writer_utility_client_browsertest, image_writer_unittest [1] http://bit.ly/2jFtsZZ BUG=690717,352442 Review-Url: https://codereview.chromium.org/2756393002 Cr-Commit-Position: refs/heads/master@{#458965} Committed: https://chromium.googlesource.com/chromium/src/+/54af805eac68ac00ef7a6a9f3ee4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/54af805eac68ac00ef7a6a9f3ee4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
