Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
294903005:
Add browser test for timezone resolve. (Closed)
|
Created:
6 years, 7 months ago by Alexander Alekseev Modified:
6 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, cpu_(ooo_6.6-7.5), mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd browser test for timezone resolve.
Also add regular expressions support to net::FakeURLFetcherFactory.
BUG=245075
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272664
Patch Set 1 #Patch Set 2 : Drop incorrect comment. #Patch Set 3 : Remove RE2 dependency. #
Total comments: 8
Patch Set 4 : Update after review. #
Total comments: 5
Patch Set 5 : Remove CreateDefaultFakeURLFetcherCreator. #
Total comments: 8
Patch Set 6 : Update after review. #Patch Set 7 : Fix tests. #
Messages
Total messages: 35 (0 generated)
TimeZone API and Geolocation API create dynamic request URL, so I've added support for regular expressions to net tests support library. Please review: willchan@ : net/* cpu@ : as OWNER of third_party/re2 : addition of third_party/re2 to net/DEPS nkostylev@ : wizard_controller*
I need to shed load. Dropping myself from reviewers. Please find another net/ reviewer.
-willchan +mmenke mmenke@ : Please review net/*
On 2014/05/20 22:10:18, alemate wrote: > TimeZone API and Geolocation API create dynamic request URL, so I've added > support for regular expressions to net tests support library. Why not override default URL instead?
On 2014/05/21 10:15:51, Nikita Kostylev wrote: > On 2014/05/20 22:10:18, alemate wrote: > > TimeZone API and Geolocation API create dynamic request URL, so I've added > > support for regular expressions to net tests support library. > > Why not override default URL instead? To simplify tests. Overriding URL requires modifications in wizard_controller, which is already rather large.
On 2014/05/21 11:12:03, alemate wrote: > On 2014/05/21 10:15:51, Nikita Kostylev wrote: > > On 2014/05/20 22:10:18, alemate wrote: > > > TimeZone API and Geolocation API create dynamic request URL, so I've added > > > support for regular expressions to net tests support library. > > > > Why not override default URL instead? > > To simplify tests. > Overriding URL requires modifications in wizard_controller, which is already > rather large. I don't see how adding one Set*ForTest method complicates code. But I'm not saying that this should be done. Just saying that it is perfectly normal to override URLs in tests specifically so that they're not dynamic but fixed URLs.
On 2014/05/21 11:32:39, Nikita Kostylev wrote: > On 2014/05/21 11:12:03, alemate wrote: > > On 2014/05/21 10:15:51, Nikita Kostylev wrote: > > > On 2014/05/20 22:10:18, alemate wrote: > > > > TimeZone API and Geolocation API create dynamic request URL, so I've added > > > > support for regular expressions to net tests support library. > > > > > > Why not override default URL instead? > > > > To simplify tests. > > Overriding URL requires modifications in wizard_controller, which is already > > rather large. > > I don't see how adding one Set*ForTest method complicates code. > But I'm not saying that this should be done. Just saying that it is perfectly > normal to override URLs in tests specifically so that they're not dynamic but > fixed URLs. Alternatively, you could subclass FakeURLFetcherFactory, or use URLRequestFilter and inject a ProtocolHandler.
On 2014/05/21 13:31:49, mmenke wrote: > On 2014/05/21 11:32:39, Nikita Kostylev wrote: > > On 2014/05/21 11:12:03, alemate wrote: > > > On 2014/05/21 10:15:51, Nikita Kostylev wrote: > > > > On 2014/05/20 22:10:18, alemate wrote: > > > > > TimeZone API and Geolocation API create dynamic request URL, so I've > added > > > > > support for regular expressions to net tests support library. > > > > > > > > Why not override default URL instead? > > > > > > To simplify tests. > > > Overriding URL requires modifications in wizard_controller, which is already > > > rather large. > > > > I don't see how adding one Set*ForTest method complicates code. > > But I'm not saying that this should be done. Just saying that it is perfectly > > normal to override URLs in tests specifically so that they're not dynamic but > > fixed URLs. > > Alternatively, you could subclass FakeURLFetcherFactory, or use URLRequestFilter > and inject a ProtocolHandler. It's tricky to subclass FakeURLFetcherFactory, because it still requires access to private members. Alternative way is to subclass TestURLFetcherFactory (and let all requests to fall though FakeURLFetcherFactory to default factory) and dispatch all requests there. It works, but adding regular expressions support to FakeURLFetcherFactory seems less complicate.
On 2014/05/21 13:48:03, alemate wrote: > On 2014/05/21 13:31:49, mmenke wrote: > > On 2014/05/21 11:32:39, Nikita Kostylev wrote: > > > On 2014/05/21 11:12:03, alemate wrote: > > > > On 2014/05/21 10:15:51, Nikita Kostylev wrote: > > > > > On 2014/05/20 22:10:18, alemate wrote: > > > > > > TimeZone API and Geolocation API create dynamic request URL, so I've > > added > > > > > > support for regular expressions to net tests support library. > > > > > > > > > > Why not override default URL instead? > > > > > > > > To simplify tests. > > > > Overriding URL requires modifications in wizard_controller, which is > already > > > > rather large. > > > > > > I don't see how adding one Set*ForTest method complicates code. > > > But I'm not saying that this should be done. Just saying that it is > perfectly > > > normal to override URLs in tests specifically so that they're not dynamic > but > > > fixed URLs. > > > > Alternatively, you could subclass FakeURLFetcherFactory, or use > URLRequestFilter > > and inject a ProtocolHandler. > > It's tricky to subclass FakeURLFetcherFactory, because it still requires access > to private members. > > Alternative way is to subclass TestURLFetcherFactory (and let all requests to > fall though FakeURLFetcherFactory to default factory) and dispatch all requests > there. It works, but adding regular expressions support to FakeURLFetcherFactory > seems less complicate. As we're in the process of making net/ usable as a library (And having spent quite a while refactoring to get rid of one dependency), I'm very reluctant to add new dependencies, particularly for one-off uses.
-cpu@ as no more DEPS modified (added to CC) On 2014/05/21 14:45:04, mmenke wrote: > On 2014/05/21 13:48:03, alemate wrote: > > On 2014/05/21 13:31:49, mmenke wrote: > > > On 2014/05/21 11:32:39, Nikita Kostylev wrote: > > > > On 2014/05/21 11:12:03, alemate wrote: > > > > > On 2014/05/21 10:15:51, Nikita Kostylev wrote: > > > > > > On 2014/05/20 22:10:18, alemate wrote: > > > > > > > TimeZone API and Geolocation API create dynamic request URL, so I've > > > added > > > > > > > support for regular expressions to net tests support library. > > > > > > > > > > > > Why not override default URL instead? > > > > > > > > > > To simplify tests. > > > > > Overriding URL requires modifications in wizard_controller, which is > > already > > > > > rather large. > > > > > > > > I don't see how adding one Set*ForTest method complicates code. > > > > But I'm not saying that this should be done. Just saying that it is > > perfectly > > > > normal to override URLs in tests specifically so that they're not dynamic > > but > > > > fixed URLs. > > > > > > Alternatively, you could subclass FakeURLFetcherFactory, or use > > URLRequestFilter > > > and inject a ProtocolHandler. > > > > It's tricky to subclass FakeURLFetcherFactory, because it still requires > access > > to private members. > > > > Alternative way is to subclass TestURLFetcherFactory (and let all requests to > > fall though FakeURLFetcherFactory to default factory) and dispatch all > requests > > there. It works, but adding regular expressions support to > FakeURLFetcherFactory > > seems less complicate. > > As we're in the process of making net/ usable as a library (And having spent > quite a while refactoring to get rid of one dependency), I'm very reluctant to > add new dependencies, particularly for one-off uses. OK. I've removed RE2 dependency from net test support library (though I still think it would be useful in tests). Please review.
https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:353: net::URLRequestStatus::SUCCESS).release(); And reason to not just use "return new FakeURLFetcher(url, d, ....);"? https://codereview.chromium.org/294903005/diff/40001/net/url_request/test_url... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/294903005/diff/40001/net/url_request/test_url... net/url_request/test_url_fetcher_factory.cc:435: FakeURLFetcherFactory::FakeURLFetcherCreator Definition order should match declaration order.
https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:353: net::URLRequestStatus::SUCCESS).release(); On 2014/05/21 18:14:10, mmenke wrote: > And reason to not just use "return new FakeURLFetcher(url, d, ....);"? "Any reason", rather.
https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:353: net::URLRequestStatus::SUCCESS).release(); On 2014/05/21 18:14:10, mmenke wrote: > And reason to not just use "return new FakeURLFetcher(url, d, ....);"? I hope this would still work after net::FakeURLFetcherFactory will be refactored. The problem is that this code should do exactly what FakeURLFetcher::CreateURLFetcher does. I need a way to ask net::FakeURLFetcherFactory to create fetcher for me. https://codereview.chromium.org/294903005/diff/40001/net/url_request/test_url... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/294903005/diff/40001/net/url_request/test_url... net/url_request/test_url_fetcher_factory.cc:435: FakeURLFetcherFactory::FakeURLFetcherCreator On 2014/05/21 18:14:10, mmenke wrote: > Definition order should match declaration order. It's after FakeURLFetcherFactory::ClearFakeResponses in both declaration and definition. Where should I move it?
https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:353: net::URLRequestStatus::SUCCESS).release(); On 2014/05/21 19:02:04, alemate wrote: > On 2014/05/21 18:14:10, mmenke wrote: > > And reason to not just use "return new FakeURLFetcher(url, d, ....);"? > > I hope this would still work after net::FakeURLFetcherFactory will be > refactored. > > The problem is that this code should do exactly what > FakeURLFetcher::CreateURLFetcher does. I need a way to ask > net::FakeURLFetcherFactory to create fetcher for me. FakeURLFetcher is a public class just so that TestURLFetcherFactory subclasses can use it. Any change to how FakeURLFetcherFactory created FakeURLFetchers would presumably add an argument to its constructor, so I really don't see what this wrapper gets us. https://codereview.chromium.org/294903005/diff/40001/net/url_request/test_url... File net/url_request/test_url_fetcher_factory.cc (right): https://codereview.chromium.org/294903005/diff/40001/net/url_request/test_url... net/url_request/test_url_fetcher_factory.cc:435: FakeURLFetcherFactory::FakeURLFetcherCreator On 2014/05/21 19:02:04, alemate wrote: > On 2014/05/21 18:14:10, mmenke wrote: > > Definition order should match declaration order. > > It's after FakeURLFetcherFactory::ClearFakeResponses in both declaration and > definition. > > Where should I move it? Ahh...you're right. Sorry, DefaultFakeURLFetcherCreator is just in the wrong order. This is fine here.
https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:353: net::URLRequestStatus::SUCCESS).release(); On 2014/05/21 19:07:49, mmenke wrote: > On 2014/05/21 19:02:04, alemate wrote: > > On 2014/05/21 18:14:10, mmenke wrote: > > > And reason to not just use "return new FakeURLFetcher(url, d, ....);"? > > > > I hope this would still work after net::FakeURLFetcherFactory will be > > refactored. > > > > The problem is that this code should do exactly what > > FakeURLFetcher::CreateURLFetcher does. I need a way to ask > > net::FakeURLFetcherFactory to create fetcher for me. > > FakeURLFetcher is a public class just so that TestURLFetcherFactory subclasses > can use it. Any change to how FakeURLFetcherFactory created FakeURLFetchers > would presumably add an argument to its constructor, so I really don't see what > this wrapper gets us. Done.
https://codereview.chromium.org/294903005/diff/60001/net/url_request/test_url... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/294903005/diff/60001/net/url_request/test_url... net/url_request/test_url_fetcher_factory.h:426: static FakeURLFetcherCreator CreateDefaultFakeURLFetcherCreator(); I don't think we need this any more? (And then you won't need my signoff, either, come to think of it)
https://codereview.chromium.org/294903005/diff/60001/net/url_request/test_url... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/294903005/diff/60001/net/url_request/test_url... net/url_request/test_url_fetcher_factory.h:426: static FakeURLFetcherCreator CreateDefaultFakeURLFetcherCreator(); On 2014/05/21 19:26:50, mmenke wrote: > I don't think we need this any more? (And then you won't need my signoff, > either, come to think of it) There is a two-lines comment, why it's needed. It's used in wizard_controller_browsertest.cc:387
https://codereview.chromium.org/294903005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:387: net::FakeURLFetcherFactory::CreateDefaultFakeURLFetcherCreator())); This is the same as FakeURLFetcherFactory(fallback_fetcher_factory_.get()), isn't it?
I've removed all modifications of net/ . Moved mmenke@ to CC list now. https://codereview.chromium.org/294903005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:387: net::FakeURLFetcherFactory::CreateDefaultFakeURLFetcherCreator())); On 2014/05/21 19:40:16, mmenke wrote: > This is the same as FakeURLFetcherFactory(fallback_fetcher_factory_.get()), > isn't it? This was the first thing I've tried, but it crashed for some reason. So I decided that it is not the same. But it actually works now, so it was my mistake. Sorry for that.
Thanks for making those changes! https://codereview.chromium.org/294903005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:387: net::FakeURLFetcherFactory::CreateDefaultFakeURLFetcherCreator())); On 2014/05/21 20:26:04, alemate wrote: > On 2014/05/21 19:40:16, mmenke wrote: > > This is the same as FakeURLFetcherFactory(fallback_fetcher_factory_.get()), > > isn't it? > > This was the first thing I've tried, but it crashed for some reason. > So I decided that it is not the same. > > But it actually works now, so it was my mistake. Sorry for that. Not a problem.
lgtm https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:290: // Otherwize sets callback and returns false. nit: Otherwise https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:290: // Otherwize sets callback and returns false. nit: Maybe revers return order? I.e. returns true if it successfully set callback. Returns false if it didn't set callback because timezone has been already resolved. https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:354: url.spec(), chromeos::DefaultTimezoneProviderURL().spec(), true)) { nit: It is possible to place here each parameter per line, will be much more readable. Is this code clang formatted? https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:500: // Enable TimeZone resolve nit: Not clear how initializing default network enables that. nit: dot at the end.
I'll commit this. https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:290: // Otherwize sets callback and returns false. On 2014/05/22 16:30:43, Nikita Kostylev wrote: > nit: Otherwise Done. https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:290: // Otherwize sets callback and returns false. On 2014/05/22 16:30:43, Nikita Kostylev wrote: > nit: Maybe revers return order? > > I.e. returns true if it successfully set callback. > Returns false if it didn't set callback because timezone has been already > resolved. Done. https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right): https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:354: url.spec(), chromeos::DefaultTimezoneProviderURL().spec(), true)) { On 2014/05/22 16:30:43, Nikita Kostylev wrote: > nit: It is possible to place here each parameter per line, will be much more > readable. > > Is this code clang formatted? Ye, it is a result of clang-format. done. https://codereview.chromium.org/294903005/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller_browsertest.cc:500: // Enable TimeZone resolve On 2014/05/22 16:30:43, Nikita Kostylev wrote: > nit: Not clear how initializing default network enables that. > nit: dot at the end. NetworkPortalDetector launches tasks when default network state is "online". TimeZone resolve is started via DelayNetworkCall(), which uses NetworkPortalDetector.
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/294903005/90001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/294903005/90001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/294903005/100001
Message was sent while issue was closed.
Change committed as 272664 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
