Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558693002:
Add WriteIsolatedOutput() functions (Closed)
DescriptionAdd WriteIsolatedOutput() functions
WriteIsolatedOutput() functions write large content into swarming isolated
output folder, which are useful to log large test data for debugging purpose.
BUG=webrtc:6732
TBR=holmer@webrtc.org
Committed: https://crrev.com/2769ec62c089218394c1fe3a5b41b9331940b68c
Cr-Commit-Position: refs/heads/master@{#15616}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Resolve review comments #
Total comments: 2
Patch Set 3 : Resolve review comments #
Messages
Total messages: 47 (31 generated)
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 ========== to ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 ==========
zijiehe@chromium.org changed reviewers: + kjellander@webrtc.org
Looks good, I just have a few comments. https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... File webrtc/test/testsupport/isolated_output.cc (right): https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... webrtc/test/testsupport/isolated_output.cc:21: "The isolated output folder provided by swarming test framework."); Please make webrtc::test::OutputPath() the default for this flag unless it's provided. https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... webrtc/test/testsupport/isolated_output.cc:35: LOG(LS_WARNING) << "filename should not be an empty string."; Change to "filename must be provided." since it might be nullptr as well https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... File webrtc/test/testsupport/isolated_output_unittest.cc (right): https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... webrtc/test/testsupport/isolated_output_unittest.cc:53: } Please add cleaning of the file and ensure it always happens. I think you either need to change ASSERT->EXPECT or use a gtest fixture class+TEST_F and implement TearDown(). Our tests shouldn't leave any files around after executing successfully (only on failures).
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/21104)
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... File webrtc/test/testsupport/isolated_output.cc (right): https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... webrtc/test/testsupport/isolated_output.cc:21: "The isolated output folder provided by swarming test framework."); On 2016/12/08 11:51:28, kjellander_webrtc wrote: > Please make webrtc::test::OutputPath() the default for this flag unless it's > provided. Done. https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... webrtc/test/testsupport/isolated_output.cc:35: LOG(LS_WARNING) << "filename should not be an empty string."; On 2016/12/08 11:51:28, kjellander_webrtc wrote: > Change to "filename must be provided." since it might be nullptr as well Done. https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... File webrtc/test/testsupport/isolated_output_unittest.cc (right): https://codereview.webrtc.org/2558693002/diff/1/webrtc/test/testsupport/isola... webrtc/test/testsupport/isolated_output_unittest.cc:53: } On 2016/12/08 11:51:28, kjellander_webrtc wrote: > Please add cleaning of the file and ensure it always happens. I think you either > need to change ASSERT->EXPECT or use a gtest fixture class+TEST_F and implement > TearDown(). > Our tests shouldn't leave any files around after executing successfully (only on > failures). Done.
lgtm with comment addressed https://codereview.webrtc.org/2558693002/diff/40001/webrtc/test/testsupport/i... File webrtc/test/testsupport/isolated_output_unittest.cc (right): https://codereview.webrtc.org/2558693002/diff/40001/webrtc/test/testsupport/i... webrtc/test/testsupport/isolated_output_unittest.cc:46: rtc::File::Open(rtc::Pathname(FLAGS_isolated_out_dir, filename)); Please declare rtc::Pathname out_file(FLAGS_isolated_out_dir, filename); then use out_file here and at line 56 to improve readability.
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2558693002/diff/40001/webrtc/test/testsupport/i... File webrtc/test/testsupport/isolated_output_unittest.cc (right): https://codereview.webrtc.org/2558693002/diff/40001/webrtc/test/testsupport/i... webrtc/test/testsupport/isolated_output_unittest.cc:46: rtc::File::Open(rtc::Pathname(FLAGS_isolated_out_dir, filename)); On 2016/12/09 06:52:09, kjellander_webrtc wrote: > Please declare > rtc::Pathname out_file(FLAGS_isolated_out_dir, filename); > then use out_file here and at line 56 to improve readability. Done.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2558693002/#ps60001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11278)
zijiehe@chromium.org changed reviewers: + palmkvist@webrtc.org
Involve Viktor for file* changes.
zijiehe@chromium.org changed reviewers: + stefan@webrtc.org
Viktor and Stefan, Do you have any concern about file* changes?
On 2016/12/13 19:27:06, Hzj_jie wrote: > Viktor and Stefan, > Do you have any concern about file* changes? lgtm
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11439)
Description was changed from ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 ========== to ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 TBR=holmer@webrtc.org ==========
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481755848658360,
"parent_rev": "c6fbcc2b92f24f3dd0d22067b4ded955dde81e17", "commit_rev":
"6612cad469da45b7ca3335715f6e21e18ab6451b"}
Message was sent while issue was closed.
Description was changed from ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 TBR=holmer@webrtc.org ========== to ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 TBR=holmer@webrtc.org Review-Url: https://codereview.webrtc.org/2558693002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 TBR=holmer@webrtc.org Review-Url: https://codereview.webrtc.org/2558693002 ========== to ========== Add WriteIsolatedOutput() functions WriteIsolatedOutput() functions write large content into swarming isolated output folder, which are useful to log large test data for debugging purpose. BUG=webrtc:6732 TBR=holmer@webrtc.org Committed: https://crrev.com/2769ec62c089218394c1fe3a5b41b9331940b68c Cr-Commit-Position: refs/heads/master@{#15616} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2769ec62c089218394c1fe3a5b41b9331940b68c Cr-Commit-Position: refs/heads/master@{#15616} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
