Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11308035:
Fix URLFetcherDownloadProgressTest to work with remote test server (Closed)
|
Created:
8 years, 1 month ago by ppi Modified:
8 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, ilevy+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, darin-cc_chromium.org, peter+watch_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix URLFetcherDownloadProgressTest to work with remote test server
The URLFetcherDownloadProgressTest assumes that the file being
downloaded is present on the machine runing the tests. This is
not true on Android, where a remote server is used instead.
Access to the file before starting the download is not necessary
to verify the consistency of data reported in download progress
callbacks, which is the aim of the test.
The patch checks if the total download size reported in the
callback remains constant, consistently with the way in which
download progress already is being verified.
BUG=161242
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170206
Patch Set 1 #Patch Set 2 : address pliard remarks #
Total comments: 4
Patch Set 3 : address wtc remarks - fix interface inheritance comments #Patch Set 4 : address wtc remarks - reintroduce file size checking on nonAndroid platforms #Patch Set 5 : Fix code formatting (braces) as advised by pliard #
Total comments: 2
Patch Set 6 : Hardcode the file size in the test #
Total comments: 8
Patch Set 7 : address wtc remarks #
Total comments: 1
Messages
Total messages: 17 (0 generated)
lgtm, thanks Przemek for fixing this!
lgtm, assuming trybots pass. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ppi@chromium.org/11308035/7003
Presubmit check for 11308035-7003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
net
willchan, could you please have a look at this? I need a lgtm from net owner to commit this patch. Philippe, kinaba - thanks a lot!
Sorry, I'm about to fly to Hawaii. Adding wtc to review or redirect to someone else.
Patch set 2 LGTM. I suggest some changes. https://codereview.chromium.org/11308035/diff/7003/net/url_request/url_fetche... File net/url_request/url_fetcher_impl_unittest.cc (left): https://codereview.chromium.org/11308035/diff/7003/net/url_request/url_fetche... net/url_request/url_fetcher_impl_unittest.cc:192: // URLFetcherDelegate We should either leave these comments unchanged, or fix all instances of these comments. Fixing these comments only in this class creates an inconsistency with the other copies of these comments above. https://codereview.chromium.org/11308035/diff/7003/net/url_request/url_fetche... net/url_request/url_fetcher_impl_unittest.cc:859: &expected_total_); Removing this file_util::GetFileSize call weakens the test because we no longer know the expected total size. Is this file_util::GetFileSize call problematic for a remote test server? I guess the file to fetch is not on the device? I wonder if the fix should be to hardcode the size of the "animate1.gif" file here.
Thanks for the remarks, wtc. I have addressed the comments as described below. Please let me know if any of you have any further remarks. https://codereview.chromium.org/11308035/diff/7003/net/url_request/url_fetche... File net/url_request/url_fetcher_impl_unittest.cc (left): https://codereview.chromium.org/11308035/diff/7003/net/url_request/url_fetche... net/url_request/url_fetcher_impl_unittest.cc:192: // URLFetcherDelegate On 2012/11/21 00:48:01, wtc wrote: > > We should either leave these comments unchanged, or fix all > instances of these comments. Fixing these comments only in > this class creates an inconsistency with the other copies > of these comments above. Thanks, I have fixed the comments throughout the entire file (patch set 3). https://codereview.chromium.org/11308035/diff/7003/net/url_request/url_fetche... net/url_request/url_fetcher_impl_unittest.cc:859: &expected_total_); On 2012/11/21 00:48:01, wtc wrote: > > Removing this file_util::GetFileSize call weakens the test > because we no longer know the expected total size. > > Is this file_util::GetFileSize call problematic for a remote > test server? I guess the file to fetch is not on the device? > > I wonder if the fix should be to hardcode the size of the > "animate1.gif" file here. You are right, thanks. I didn't feel comfortable about hardcoding the file size in the test, so I made it being verified only on non-Android platforms (patch set 4).
Review comments on patch set 5:
Hi ppi: it is a holiday in the US today, so I only took a
quick look and didn't do a complete code review.
I think we should avoid platform-specific differences in our
tests. If it is not important to test the 'expected_total'
part, I am fine with your original approach of just testing
the total size stays the same. If you think it is useful to
test the 'expected_total', then I think it's simpler to just
hardcode the size of animate1.gif.
Also, I'd like you to find out what is the most common
format of the comments for methods overriden from a base
class. I have seen
// FooBaseClass methods:
// FooBaseClass implementation:
But I don't remember seeing just
// FooBaseClass:
Is this style also common in the source tree? In any
case, your change to use the correct base class name
in those comments is a big improvement. Thanks.
https://codereview.chromium.org/11308035/diff/12006/net/url_request/url_fetch...
File net/url_request/url_fetcher_impl_unittest.cc (right):
https://codereview.chromium.org/11308035/diff/12006/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:878: static const char
kFileToFetch[] = "animate1.gif";
Since we hardcoded the file name here, I think it is fine to
also hardcode its file size.
https://codereview.chromium.org/11308035/diff/12006/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:883: &file_size));
I think you missed the trailing '_' for 'file_size_' here.
On 2012/11/22 18:58:02, wtc wrote: > Review comments on patch set 5: > > Hi ppi: it is a holiday in the US today, so I only took a > quick look and didn't do a complete code review. > > I think we should avoid platform-specific differences in our > tests. If it is not important to test the 'expected_total' > part, I am fine with your original approach of just testing > the total size stays the same. If you think it is useful to > test the 'expected_total', then I think it's simpler to just > hardcode the size of animate1.gif. > > Also, I'd like you to find out what is the most common > format of the comments for methods overriden from a base > class. I have seen > // FooBaseClass methods: > > // FooBaseClass implementation: > > But I don't remember seeing just > // FooBaseClass: > > Is this style also common in the source tree? In any > case, your change to use the correct base class name > in those comments is a big improvement. Thanks. > > https://codereview.chromium.org/11308035/diff/12006/net/url_request/url_fetch... > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/11308035/diff/12006/net/url_request/url_fetch... > net/url_request/url_fetcher_impl_unittest.cc:878: static const char > kFileToFetch[] = "animate1.gif"; > > Since we hardcoded the file name here, I think it is fine to > also hardcode its file size. > > https://codereview.chromium.org/11308035/diff/12006/net/url_request/url_fetch... > net/url_request/url_fetcher_impl_unittest.cc:883: &file_size)); > > I think you missed the trailing '_' for 'file_size_' here. Regarding comments for overridden methods, the style guide could be clearer. It says 'When you derive from a base class, group any overriding functions in your header file in one *labeled* section'. In practice the most common format is '// SuperClass:': $ git grep -E '// [a-zA-Z0-9]+:$' | wc -l 2133 $ git grep -E '// [a-zA-Z0-9]+ overrides:$' | wc -l 198 $ git grep -E '// [a-zA-Z0-9]+ methods:$' | wc -l 122 $ git grep -E '// [a-zA-Z0-9]+ implementation:$' | wc -l 251
Thanks for further remarks, wtc. I have followed your suggestion and hardcoded the file size in patch set 6. Please let me know if the patch is ok with you. Thanks for the insightful statistics, Philippe.
Patch set 6 LGTM. ppi: I requested some changes. Because of our time zone difference, you can commit the CL after making those changes, without waiting for my review. Just upload a new patch set before you commit it. Thanks. https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... File net/url_request/url_fetcher_impl_unittest.cc (left): https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:172: // URLFetcherDelegate Strictly speaking, these "URLFetcherDelegate" comments for OnURLFetchComplete are correct because the OnURLFetchComplete method ultimately comes from URLFetcherDelegate. I don't know what is the right class to use in these comments though... net/socket/tcp_client_socket_win.h uses the most specific class name in these method override comments. I would probably do that, too. I'll let you decide. https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:200: // Data returned by the previous callback. "Data" => "Download progress" or "Number of bytes" https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:206: int64 expected_total_size_; 1. We don't need first_call_ any more. Since previous_progress_ is initialized to 0, EXPECT_LE(previous_progress_, current); (on line 468) is also true for the first call. 2. We can reduce the changes in this CL by changing expected_total_size_ back to its original name expected_total_. https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:471: first_call_ = false; If we remove first_call_ and use the original expected_total_ name, I believe this method doesn't need any change.
On 2012/11/23 09:06:05, Philippe wrote:
>
> Regarding comments for overridden methods, the style guide could be clearer.
It
> says 'When you derive from a base class, group any overriding functions in
your
> header file in one *labeled* section'. In practice the most common format is
'//
> SuperClass:':
>
> $ git grep -E '// [a-zA-Z0-9]+:$' | wc -l
> 2133
>
> $ git grep -E '// [a-zA-Z0-9]+ overrides:$' | wc -l
> 198
>
> $ git grep -E '// [a-zA-Z0-9]+ methods:$' | wc -l
> 122
>
> $ git grep -E '// [a-zA-Z0-9]+ implementation:$' | wc -l
> 251
ppi: this review comment is informational only. I am not suggesting
any change.
pilard: thank you for the statistics.
I ran these commands inside src/net and got different trends among
the last three styles:
FooClass overrides: 2
FooClass methods: 32
FooClass implementation: 12
The first style is the highest, but it has a lot of false hits like
./tools/flip_server/create_listener.h:// Summary:
./tools/flip_server/create_listener.h:// Args:
./tools/flip_server/epoll_server.h: // Summary:
./tools/flip_server/epoll_server.h: // Args:
./tools/flip_server/epoll_server.h: // Summary:
./tools/flip_server/epoll_server.h: // Args:
./tools/flip_server/epoll_server.h: // Summary:
./tools/flip_server/epoll_server.h: // Args:
I don't know how to filter out these false hits, so it's not clear
whether
// FooClass:
or
// FooClass methods:
is the most common inside src/net.
Thanks for a careful review, wtc! I have addressed your most recent remarks in patch set 7, and I intend to commit the patch when the try bots flash green. https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... File net/url_request/url_fetcher_impl_unittest.cc (left): https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:172: // URLFetcherDelegate I agree - I think that precise "URLFetcherDelegate:" comments make the class interface more readable. I have restored them in patch set 7. On 2012/11/26 23:19:06, wtc wrote: > > Strictly speaking, these "URLFetcherDelegate" comments for > OnURLFetchComplete are correct because the OnURLFetchComplete > method ultimately comes from URLFetcherDelegate. > > I don't know what is the right class to use in these > comments though... net/socket/tcp_client_socket_win.h uses > the most specific class name in these method override > comments. I would probably do that, too. I'll let you > decide. https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:200: // Data returned by the previous callback. Absolutely, fixed. On 2012/11/26 23:19:06, wtc wrote: > > "Data" => "Download progress" or "Number of bytes" https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:206: int64 expected_total_size_; I find "expected_total_size_" more descriptive and clear than "expected_total_", but I am flexible on that. Fixed both in patch set 7. On 2012/11/26 23:19:06, wtc wrote: > > 1. We don't need first_call_ any more. Since previous_progress_ > is initialized to 0, > EXPECT_LE(previous_progress_, current); > (on line 468) is also true for the first call. > > 2. We can reduce the changes in this CL by changing > expected_total_size_ back to its original name expected_total_. > https://codereview.chromium.org/11308035/diff/12/net/url_request/url_fetcher_... net/url_request/url_fetcher_impl_unittest.cc:471: first_call_ = false; Removed "first_call_". I did rename "current" to "progress", to pair well with "previous_progress_" though. On 2012/11/26 23:19:06, wtc wrote: > > If we remove first_call_ and use the original expected_total_ > name, I believe this method doesn't need any change.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ppi@chromium.org/11308035/10011
Message was sent while issue was closed.
Change committed as 170206
Message was sent while issue was closed.
Patch set 7 LGTM. Thanks! https://codereview.chromium.org/11308035/diff/10011/net/url_request/url_fetch... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/11308035/diff/10011/net/url_request/url_fetch... net/url_request/url_fetcher_impl_unittest.cc:474: const URLFetcher* source, int64 progress, int64 total) { I agree that |progress| seems more informative than |current|. The problem with renaming the argument only here is that all the other OnURLFetchDownloadProgress declarations and definitions still use the old argument name, so this change created an inconsistency. Consistency is valued in our Style Guide. Can you look into if it is worth the trouble to rename that argument globally in the source tree? Thanks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
