Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1216643002:
Added unittests for SystemLogUploader. (Closed)
|
Created:
5 years, 6 months ago by Polina Bondarenko Modified:
5 years, 4 months ago Reviewers:
Andrew T Wilson (Slow) CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded unittests for SystemLogUploader.
Time frequency and header constants in SystemLogUploader became public.
BUG=471888
Committed: https://crrev.com/2bc0cb44b3fb3f963859a1772e17547f88bfd75c
Cr-Commit-Position: refs/heads/master@{#342086}
Patch Set 1 : #Patch Set 2 : Fixed according to the main log uploader CL. #
Total comments: 16
Patch Set 3 : Fixed comments. #
Total comments: 8
Patch Set 4 : Fixed deps since the SystemLogUploadJob was removed, fixed comments. #Patch Set 5 : Fixed unittests according to the last system log uploader changes. #Patch Set 6 : Fixed according to the last log uploader review changes. #Patch Set 7 : #Patch Set 8 : Rebased patch. #
Messages
Total messages: 41 (22 generated)
Message was sent while issue was closed.
Patchset #1 (id:1) has been deleted
pbond@chromium.org changed reviewers: + atwilson@chromium.org
Hi Andrew, there are unit tests for SystemLogUploader and SystemLogUploadJob. could you please take a look at chrome/browser/chromeos/policy/system_log_upload_job.h chrome/browser/chromeos/policy/system_log_upload_job.cc chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc chrome/browser/chromeos/policy/system_log_uploader.h chrome/browser/chromeos/policy/system_log_uploader.cc chrome/browser/chromeos/policy/system_log_uploader_unittest.cc
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:92: base::Bind(failed_callback_); What does this line do? https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc (right): https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:64: EXPECT_EQ(kMockUploadUrl, upload_url_.spec()); What is this testing, exactly? And why are we testing it here instead of in the constructor? It just seems weird because you're basically just testing that your tests only pass in kMockUploadUrl to the constructor of your test class -- you aren't testing any logic in the real classes, right? https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:69: return; This is OK as is, but usually I prefer: if (error_code_) { HANDLE_ERROR } else { HANDLE NO ERROR } rather than if (error_code_) { HANDLE_ERROR() return; } HANDLE_NO_ERROR() https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:81: system_logs->push_back(std::make_pair(file_path, file_path)); Is there any code that tests that this file data is correctly processed and included in upload data? https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:127: class SystemLogUploadTest : public ash::test::AshTestBase { Why are we deriving from AshTestBase? I don't think we need anything in there? https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:162: base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this), Seems like it should be an error for this second callback to ever be invoked - maybe you should have some kind of callback that just has a FAIL() statement if it's ever called? https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:177: base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this), ditto here, success callback shouldn't be called, right? https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right): https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:162: } Good, you've covered the cases of making sure that the right retry times are used. Are there other tests we should also include? maybe also check to make sure that the right headers are being set and the right data is being sent up to the upload job?
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:130001) has been deleted
Patchset #3 (id:150001) has been deleted
Fixed comments. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job.cc (right): https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job.cc:92: base::Bind(failed_callback_); On 2015/07/03 16:19:59, Andrew T Wilson wrote: > What does this line do? Removed. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc (right): https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:64: EXPECT_EQ(kMockUploadUrl, upload_url_.spec()); On 2015/07/03 16:19:59, Andrew T Wilson wrote: > What is this testing, exactly? And why are we testing it here instead of in the > constructor? It just seems weird because you're basically just testing that your > tests only pass in kMockUploadUrl to the constructor of your test class -- you > aren't testing any logic in the real classes, right? Yes, removed. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:69: return; On 2015/07/03 16:20:00, Andrew T Wilson wrote: > This is OK as is, but usually I prefer: > > if (error_code_) { > HANDLE_ERROR > } else { > HANDLE NO ERROR > } > > rather than > > if (error_code_) { > HANDLE_ERROR() > return; > } > HANDLE_NO_ERROR() Done. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:81: system_logs->push_back(std::make_pair(file_path, file_path)); On 2015/07/03 16:20:00, Andrew T Wilson wrote: > Is there any code that tests that this file data is correctly processed and > included in upload data? Added. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:127: class SystemLogUploadTest : public ash::test::AshTestBase { On 2015/07/03 16:19:59, Andrew T Wilson wrote: > Why are we deriving from AshTestBase? I don't think we need anything in there? Done. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:162: base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this), On 2015/07/03 16:19:59, Andrew T Wilson wrote: > Seems like it should be an error for this second callback to ever be invoked - > maybe you should have some kind of callback that just has a FAIL() statement if > it's ever called? Done. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc:177: base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this), On 2015/07/03 16:20:00, Andrew T Wilson wrote: > ditto here, success callback shouldn't be called, right? Done. https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right): https://codereview.chromium.org/1216643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:162: } On 2015/07/03 16:20:00, Andrew T Wilson wrote: > Good, you've covered the cases of making sure that the right retry times are > used. > > Are there other tests we should also include? maybe also check to make sure that > the right headers are being set and the right data is being sent up to the > upload job? I decided to test it separately. Thus here is the unit tests for system log uploader (checking times and appropriate success/failure callbacks), and the unittests that check the headers of UploadJob are added to the system_log_upload_job_unittest.cc file.
mostly LG with a few questions/nits https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right): https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:21: scoped_ptr<UploadJob> CreateUploadJob(const GURL&, Maybe explain that neither of these should ever be called (since you are just testing retry logic)? Can you explain why you aren't just passing nullptr in as the SystemLogUploadJob::Delegate (why do we need this class at all if this object should never be used)? https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:50: void Run() override { callback.Run(); } This is OK, but would be even better if we could easily post a task with the response to better match the behavior of the real class (which never calls the callback from within the Run() method. But I'm fine with this. BTW, would the callback free this class? Why don't we need to call ResetAndReturn() here? https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:91: // The previours task scheduled new log upload task. nit: The previous task should have uploaded another log upload task. https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:139: SystemLogUploader::kDefaultUploadDelayMs)); Maybe highlight that we are using the upload_delay and not the error_delay here because there's just one retry?
Patchset #4 (id:190001) has been deleted
Fixed comments, removed system_log_upload_job_unittests.cc since system log upload job doesn't exist anymore. https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right): https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:21: scoped_ptr<UploadJob> CreateUploadJob(const GURL&, On 2015/07/17 15:59:28, Andrew T Wilson wrote: > Maybe explain that neither of these should ever be called (since you are just > testing retry logic)? > > Can you explain why you aren't just passing nullptr in as the > SystemLogUploadJob::Delegate (why do we need this class at all if this object > should never be used)? Changed the system log upload logic here: https://codereview.chromium.org/1193333017/ Now MockUploadJob is created for every scheduled log upload. https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:50: void Run() override { callback.Run(); } On 2015/07/17 15:59:28, Andrew T Wilson wrote: > This is OK, but would be even better if we could easily post a task with the > response to better match the behavior of the real class (which never calls the > callback from within the Run() method. But I'm fine with this. > > BTW, would the callback free this class? Why don't we need to call > ResetAndReturn() here? Removed this MockSystemLogUploadJob at all. https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:91: // The previours task scheduled new log upload task. On 2015/07/17 15:59:28, Andrew T Wilson wrote: > nit: The previous task should have uploaded another log upload task. Done. https://codereview.chromium.org/1216643002/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:139: SystemLogUploader::kDefaultUploadDelayMs)); On 2015/07/17 15:59:28, Andrew T Wilson wrote: > Maybe highlight that we are using the upload_delay and not the error_delay here > because there's just one retry? Done.
lgtm
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1216643002/#ps270001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216643002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216643002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216643002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216643002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216643002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216643002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216643002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216643002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1216643002/#ps290001 (title: "Rebased patch.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216643002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216643002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216643002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216643002/290001
Message was sent while issue was closed.
Committed patchset #8 (id:290001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2bc0cb44b3fb3f963859a1772e17547f88bfd75c Cr-Commit-Position: refs/heads/master@{#342086} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
