Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654893002:
Add unit tests for RedirectToFileResourceHandler. (Closed)
|
Created:
3 years, 11 months ago by mmenke Modified:
3 years, 11 months ago Reviewers:
Charlie Harrison CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, Randy Smith (Not in Mondays) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unit tests for RedirectToFileResourceHandler.
It had some integration tests, but no real unit tests. Also fix its
behavior when a write synchronously succeeds, which would cause it to
resume a request without first pausing it. The API for writing to files
allows sync completion, though it currently never happens, so was not a
visible bug.
BUG=679483, 659317
Review-Url: https://codereview.chromium.org/2654893002
Cr-Commit-Position: refs/heads/master@{#446472}
Committed: https://chromium.googlesource.com/chromium/src/+/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd
Patch Set 1 #Patch Set 2 : More tests, cleanup #Patch Set 3 : fix #Patch Set 4 : . #Patch Set 5 : ... #Patch Set 6 : Cleanup #Patch Set 7 : More cleanup #Patch Set 8 : Oops #
Total comments: 1
Patch Set 9 : Oops (x2) #Patch Set 10 : Remove old tests, update comment #
Total comments: 25
Patch Set 11 : Response to comments #
Total comments: 2
Patch Set 12 : Add comment #Patch Set 13 : Revise #
Messages
Total messages: 57 (46 generated)
The CQ bit was checked by mmenke@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mmenke@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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 mmenke@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 checked by mmenke@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...
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by mmenke@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 mmenke@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 checked by mmenke@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 checked by mmenke@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...
Patchset #8 (id:180001) has been deleted
mmenke@chromium.org changed reviewers: + csharrison@chromium.org
Think this one's ready for review. I'll rebase the other one on top if it later this afternoon. https://codereview.chromium.org/2654893002/diff/200001/content/browser/loader... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/2654893002/diff/200001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler.cc:362: if (did_defer_ && !completed_during_write_ && !BufIsFull()) { Note that most of the changes to this file aren't necessary. The one thing that is needed is the addition of did_defer_ before calling Resume, to fix https://crbug.com/679483. The other changes are mostly to make it match what I want to have after the other CL. The Resume() here will also be called on the no-defer path, for instance.
The CQ bit was checked by mmenke@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 checked by mmenke@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...
https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... File content/browser/loader/resource_loader_unittest.cc (left): https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/resource_loader_unittest.cc:1444: class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest { Seems reasonable to remove these tests - these are a little more integrationy, but not sure they get us anything over the new unit tests and browser_tests do. They're also harder to work with because they use a "MockFileStream" which wraps a real FileStream, instead of a purely test class (That's also why I opted to create new tests, rather than beef up these).
The CQ bit was checked by mmenke@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.
Looks good, just minor comments. I am impressed with how readable the tests are, great job. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... File content/browser/loader/redirect_to_file_resource_handler_unittest.cc (right): https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:50: // RedirectToFileResourceHandler::IsBufferFull(). I think it's pronounced "BufIsFull" https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:61: // return errors and complate operations synchronously or asynchronously. s/complate/complete https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:96: base::BindRepeating(&MockFileStream::SetClosedAndRunCallback, Why does this need to be a BindRepeating? https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:125: EXPECT_GE(buf_len, 0); EXPECT_GT if 0 byte writes aren't allowed. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:210: new TestResourceHandler()); nit: MakeUnique is preferred over bare "new" https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:252: static std::string CreateTestData(size_t length) { Optional: We always want to set the expected written data equal to the created test data. Maybe we should merge those methods? Not a big deal, but I expect it may be easy to write a test and forget to expect we write it. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:253: std::string test_data; nit: test_data.reserve(length) for (size_t i = 0; i < length; ++i) test_data.push_back(static_cast<char>(i % 256)); https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:259: void CreateTemporaryFileStream( Seems strange to call this "CreateTemporaryFileStream" when all it is doing is storing the callback. I don't have any great suggestions though. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:286: // If this is an async test, |test_handler_| will defer the nit: This comment block can fit more words on this line and below. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:422: CompleteRequestSuccessfully(test_data.size()); Can we EXPECT that we did N separate writes? Ditto for a bunch of tests. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:428: CreateTestData(RedirectToFileResourceHandler::kInitialReadBufSize); expect kInitialReadBufSize > kMaxInitialSyncReadSize https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:560: EXPECT_EQ(read_size, redirect_to_file_handler_->GetBufferSizeForTesting()); Do we really want the fact these are exactly equal to be tested as this classes contract? I think I would prefer EXPECT_LE, in case growable io buffer changes grow/capacity semantics or something. Feel free to disagree though.
Oh, I was also wondering if allocating so many big strings will be bad for performance of the unit test. I wonder if GTest has a way to set suite-scoped data?
On 2017/01/26 16:45:42, Charlie Harrison wrote: > Oh, I was also wondering if allocating so many big strings will be bad for > performance of the unit test. I wonder if GTest has a way to set suite-scoped > data? I think generally only a limited number of randomly selected tests are run per-process? At least only 10 are run locally, though maybe the bots do something different. Experimentally, the biggest test (The one that expands the buffer size) takes like 30 milliseconds in debug build on Windows, so don't think it's a big issue, in practice. The multiple sequential reads test did run into performance issues when I had it reading one byte at a time (Took a couple seconds), so I changed that.
The CQ bit was checked by mmenke@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...
Thanks for the great feedback and speedy review! I didn't make all of your suggested changes, but think they're all reasonable suggestions, happy to talk through the ones I didn't follow up one. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... File content/browser/loader/redirect_to_file_resource_handler_unittest.cc (right): https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:50: // RedirectToFileResourceHandler::IsBufferFull(). On 2017/01/26 16:44:48, Charlie Harrison wrote: > I think it's pronounced "BufIsFull" Done. That might be how you city slickers pronounce it, but we country boys... (No, I'm not really a country boy) https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:61: // return errors and complate operations synchronously or asynchronously. On 2017/01/26 16:44:47, Charlie Harrison wrote: > s/complate/complete Done. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:96: base::BindRepeating(&MockFileStream::SetClosedAndRunCallback, On 2017/01/26 16:44:47, Charlie Harrison wrote: > Why does this need to be a BindRepeating? Because ReturnResult takes net::CompletionCallbacks, which are repeating callbacks. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:125: EXPECT_GE(buf_len, 0); On 2017/01/26 16:44:47, Charlie Harrison wrote: > EXPECT_GT if 0 byte writes aren't allowed. Done. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:210: new TestResourceHandler()); On 2017/01/26 16:44:47, Charlie Harrison wrote: > nit: MakeUnique is preferred over bare "new" Done. Note that I'm not using auto blah = MakeUnique<Blah>, as I find it makes code more obscure, so I'm writing out the type twice (Which is why I didn't use MakeUnique in the first place). I'm sure some day I'll have a showdown with someone over that pattern. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:252: static std::string CreateTestData(size_t length) { On 2017/01/26 16:44:47, Charlie Harrison wrote: > Optional: We always want to set the expected written data equal to the created > test data. Maybe we should merge those methods? Not a big deal, but I expect it > may be easy to write a test and forget to expect we write it. There is are two tests that doesn't, actually. I've gone ahead and set it to this by default, though. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:253: std::string test_data; On 2017/01/26 16:44:47, Charlie Harrison wrote: > nit: > test_data.reserve(length) > for (size_t i = 0; i < length; ++i) > test_data.push_back(static_cast<char>(i % 256)); Done. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:259: void CreateTemporaryFileStream( On 2017/01/26 16:44:47, Charlie Harrison wrote: > Seems strange to call this "CreateTemporaryFileStream" when all it is doing is > storing the callback. I don't have any great suggestions though. True, but on the other hand, it is the CreateTemporaryFileStreamCallback. I've called it SetCreate*Callback, and added a comment. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:286: // If this is an async test, |test_handler_| will defer the On 2017/01/26 16:44:47, Charlie Harrison wrote: > nit: This comment block can fit more words on this line and below. Done. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:422: CompleteRequestSuccessfully(test_data.size()); On 2017/01/26 16:44:47, Charlie Harrison wrote: > Can we EXPECT that we did N separate writes? Ditto for a bunch of tests. What does that get us? You just want to verify that the logic limiting sizes of writes work? Note that this actually does 51 writes, since kInitialReadBufSize is not divisible by 50, but test shouldn't depend on that, and logic to work around it seems a bit much. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:428: CreateTestData(RedirectToFileResourceHandler::kInitialReadBufSize); On 2017/01/26 16:44:47, Charlie Harrison wrote: > expect kInitialReadBufSize > kMaxInitialSyncReadSize I'm not seeing why? I suppose we could do a compile assert where we calculate kMaxInitialSyncReadSize up top, but not sure what it gets us. https://codereview.chromium.org/2654893002/diff/240001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:560: EXPECT_EQ(read_size, redirect_to_file_handler_->GetBufferSizeForTesting()); On 2017/01/26 16:44:47, Charlie Harrison wrote: > Do we really want the fact these are exactly equal to be tested as this classes > contract? I think I would prefer EXPECT_LE, in case growable io buffer changes > grow/capacity semantics or something. Feel free to disagree though. I think EQ is the best way of proving that the limit is respected (i.e., we run until it reaches the max, then run once more, and see that it doesn't grow). I'm keeping it as-is, but happy to talk through this to reach something we both like. Also note that with how we calculate read_sizes, the expected_written_data check would fail without this, if we just saturated the buffer each time instead, so we'd need another workaround there. So we'd need to keep a weak pointer to the file stream handle to change how we do that.
I'm okay with your choices. Let me scan the deleted tests to make sure we aren't losing coverage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with request for comment https://codereview.chromium.org/2654893002/diff/260001/content/browser/loader... File content/browser/loader/redirect_to_file_resource_handler_unittest.cc (right): https://codereview.chromium.org/2654893002/diff/260001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:55: enum class CompletionMode { Can you comment on this class, and what the differences of tests that run in SYNC/ASYNC mode are? This could also be a nice place for a high level comment like "All the below tests are run in both SYNC and ASYNC mode..."
Thanks! https://codereview.chromium.org/2654893002/diff/260001/content/browser/loader... File content/browser/loader/redirect_to_file_resource_handler_unittest.cc (right): https://codereview.chromium.org/2654893002/diff/260001/content/browser/loader... content/browser/loader/redirect_to_file_resource_handler_unittest.cc:55: enum class CompletionMode { On 2017/01/26 18:53:34, Charlie Harrison wrote: > Can you comment on this class, and what the differences of tests that run in > SYNC/ASYNC mode are? This could also be a nice place for a high level comment > like "All the below tests are run in both SYNC and ASYNC mode..." Done. You're right, definitely needed a comment.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2654893002/#ps300001 (title: "Revise")
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": 300001, "attempt_start_ts": 1485461792131940,
"parent_rev": "f2b51b2a425b8ee18e11d2c8edee1d8e543ad855", "commit_rev":
"9874e16e6ccd443a33d4207619e87c6fc6ad1dcd"}
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for RedirectToFileResourceHandler. It had some integration tests, but no real unit tests. Also fix its behavior when a write synchronously succeeds, which would cause it to resume a request without first pausing it. The API for writing to files allows sync completion, though it currently never happens, so was not a visible bug. BUG=679483,659317 ========== to ========== Add unit tests for RedirectToFileResourceHandler. It had some integration tests, but no real unit tests. Also fix its behavior when a write synchronously succeeds, which would cause it to resume a request without first pausing it. The API for writing to files allows sync completion, though it currently never happens, so was not a visible bug. BUG=679483,659317 Review-Url: https://codereview.chromium.org/2654893002 Cr-Commit-Position: refs/heads/master@{#446472} Committed: https://chromium.googlesource.com/chromium/src/+/9874e16e6ccd443a33d4207619e8... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:300001) as https://chromium.googlesource.com/chromium/src/+/9874e16e6ccd443a33d4207619e8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
