Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1090003003:
Clean up URLFetcher unit tests, part 6. (Closed)
DescriptionClean up URLFetcher unit tests, part 6.
Convert URLFetcher[Upload/Download]Progress tests to use
the shared test fixture. Also add a bonus test where we
cancel/delete a fetcher in an upload progress callback.
BUG=471069
Committed: https://crrev.com/44bc8c87668304926a28b333af7c398a7cf2735e
Cr-Commit-Position: refs/heads/master@{#325565}
Patch Set 1 #Patch Set 2 : Switch from callbacks to subclasses (More code, less cruft in shared class) #
Total comments: 16
Patch Set 3 : Response to comments #Patch Set 4 : Update comments #
Total comments: 2
Patch Set 5 : remove comma #Patch Set 6 : Merge #Messages
Total messages: 12 (4 generated)
mmenke@chromium.org changed reviewers: + davidben@chromium.org
After this, there's a test that reuses the same URLFetcher twice (Ick!), a couple that cancel requests and check for context destruction (Which are a bit of a pain due to threading issues), and the HTTPS test.
https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:107: EXPECT_FALSE(did_complete_); Optional: I think it might be marginally better to remove EXPECT_FALSE(did_complete_) and put EXPECT_FALSE(fetcher->did_complete()) in the delegates. Not a huge deal, but it's not obvious from the name that CancelFetch isn't allowed to be called after the request completes. (I think normally it'd just be a no-op or something.) https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:126: // because some tests retry on 5xx responses. Nit: I'd maybe say Note that the current progress may be greater than the previous [...] so it's describing what may happen rather than explaining why we can't check something. (Seems silly that we don't have an OnRestart hook. If you pay attention to progress bits, you probably care about those.) https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:135: } Nit: much as I prefer putting in curlies, I think it's Chromium style to omit them. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:142: // because some tests retry uploads on 5xx responses. Ditto. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:885: MaybeAppendChunk(); The old test didn't require that OnURLFetchUploadProgress is called before any chunks are added. It's actually kinda weird that it is, I think. It's because this callback is called polling (!!!!) and current_upload_bytes_ is initialized to -1, so it thinks -1 to 0 wasn't a no-op. I'm torn on whether we shouldn't be testing based on this because it's weird or we should because something might be relying on it. Maybe both. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:891: num_chunks_appended_ < kNumChunks) { I'm assuming doing 5 chunks was an intentional change from the old code (which was slightly weird...). https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:898: int64 bytes_sent() const { return num_chunks_appended_ * chunk_.size(); } Nit: s/bytes_sent/bytes_appended/? Or bytes_prepared? It's not actually the number of bytes that were sent. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:914: // Since uploads progress uses a timer, may not receive any notification, Nit: Since upload progress -> Since upload progress may not -> the delegate may not
Patchset #3 (id:40001) has been deleted
Thanks for giving useful feedback for these mind-numbing reviews! Know there can be a tendency to be lazy on them (And that's probably also affected the quality of my code here, too...I just want to finish them, dangit...) https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:107: EXPECT_FALSE(did_complete_); On 2015/04/16 20:55:35, David Benjamin wrote: > Optional: I think it might be marginally better to remove > EXPECT_FALSE(did_complete_) and put EXPECT_FALSE(fetcher->did_complete()) in the > delegates. Not a huge deal, but it's not obvious from the name that CancelFetch > isn't allowed to be called after the request completes. (I think normally it'd > just be a no-op or something.) The tests are already checking that, actually, so I just removed the line. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:126: // because some tests retry on 5xx responses. On 2015/04/16 20:55:35, David Benjamin wrote: > Nit: I'd maybe say > > Note that the current progress may be greater than the previous [...] > > so it's describing what may happen rather than explaining why we can't check > something. (Seems silly that we don't have an OnRestart hook. If you pay > attention to progress bits, you probably care about those.) Done. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:135: } On 2015/04/16 20:55:35, David Benjamin wrote: > Nit: much as I prefer putting in curlies, I think it's Chromium style to omit > them. Done. I've had people suggest using them for macros, which is why I put them here...Of course, our macros work that way, anyways. I don't really feel strongly, either way. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:142: // because some tests retry uploads on 5xx responses. On 2015/04/16 20:55:35, David Benjamin wrote: > Ditto. Done. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:885: MaybeAppendChunk(); On 2015/04/16 20:55:35, David Benjamin wrote: > The old test didn't require that OnURLFetchUploadProgress is called before any > chunks are added. It's actually kinda weird that it is, I think. It's because > this callback is called polling (!!!!) and current_upload_bytes_ is initialized > to -1, so it thinks -1 to 0 wasn't a no-op. > > I'm torn on whether we shouldn't be testing based on this because it's weird or > we should because something might be relying on it. Maybe both. Done. I agree it's weird, I just noticed it worked, and it saved me the massive effort of starting the request, appending the chunk, and then waiting on it. Oh, the humanity! https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:891: num_chunks_appended_ < kNumChunks) { On 2015/04/16 20:55:35, David Benjamin wrote: > I'm assuming doing 5 chunks was an intentional change from the old code (which > was slightly weird...). Yea, sending just 2 seemed insufficient. https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:898: int64 bytes_sent() const { return num_chunks_appended_ * chunk_.size(); } On 2015/04/16 20:55:35, David Benjamin wrote: > Nit: s/bytes_sent/bytes_appended/? Or bytes_prepared? It's not actually the > number of bytes that were sent. Done, absolutely right! https://codereview.chromium.org/1090003003/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:914: // Since uploads progress uses a timer, may not receive any notification, On 2015/04/16 20:55:35, David Benjamin wrote: > Nit: Since upload progress -> Since upload progress > may not -> the delegate may not Done.
lgtm https://codereview.chromium.org/1090003003/diff/80001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1090003003/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:913: // notification, otherwise. Nit: I think this comma is superfluous? I dunno, English.
https://codereview.chromium.org/1090003003/diff/80001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1090003003/diff/80001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:913: // notification, otherwise. On 2015/04/16 21:49:54, David Benjamin wrote: > Nit: I think this comma is superfluous? I dunno, English. I think the rule is it's needed if it's at the start of a sentence, but not at the end? Anyhow, removed.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1090003003/#ps120001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1090003003/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/44bc8c87668304926a28b333af7c398a7cf2735e Cr-Commit-Position: refs/heads/master@{#325565} |
