Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1989363007:
Move predictor dns unit tests to browser tests (Closed)
|
Created:
4 years, 7 months ago by Charlie Harrison Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove predictor dns unit tests to browser tests
This patch moves some unit tests for the predictor to browser tests. This
is in preparation for a move where the predictor will call out to //content
for preresolve requests.
The patch also refactors how DNS browser tests work, where
we query off the predictor's observer.
BUG=610750, 469120
Committed: https://crrev.com/0597b217f6e2400796536e70ab3413743f4abf80
Cr-Commit-Position: refs/heads/master@{#396444}
Patch Set 1 #Patch Set 2 : Fix some races #Patch Set 3 : Hopefully fixed 469120 #
Total comments: 4
Patch Set 4 : Responded to rdsmith@s comments #Patch Set 5 : #Patch Set 6 : #
Total comments: 14
Patch Set 7 : eroman@ review #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. BUG=610750 ========== to ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. BUG=610750,469120 ==========
csharrison@chromium.org changed reviewers: + rdsmith@chromium.org
Randy, do you mind taking a look at this one?
I'm out of date on overall philosophy of testing at the content level, but when I was up to date I remember a) we preferred unit tests to browser tests because they were much less flaky, and b) someone had wired up a unit test framework that was *almost* a browser test (everything but the UI?) which was where they were encouraging people to put new tests. Does this ring bells to you and have you incorporated it into your thinking? If not, it might be worthwhile asking a content/ guru what the (current :-}) right thing to do at the content testing level is. (I'm happy to do the nuts and bolts of the review, I just want to make sure the overall approach is right first.)
On 2016/05/21 21:04:21, Randy Smith - Not in Fridays wrote: > I'm out of date on overall philosophy of testing at the content level, but when > I was up to date I remember a) we preferred unit tests to browser tests because > they were much less flaky, and b) someone had wired up a unit test framework > that was *almost* a browser test (everything but the UI?) which was where they > were encouraging people to put new tests. Does this ring bells to you and have > you incorporated it into your thinking? If not, it might be worthwhile asking a > content/ guru what the (current :-}) right thing to do at the content testing > level is. > > (I'm happy to do the nuts and bolts of the review, I just want to make sure the > overall approach is right first.) The DNS preresolve code I'm planning on adding to //content is scoped to the resource dispatcher host. To test it with unit test we'll have to stand up a mock RDHI. I talked with Matt about it and he recommended switching to browser tests here, instead of mocking RDHI. I know lots of content unit tests that do navigations / UI mocking but I'm not sure if any of them instantiate an RDHI. I can look into it. Keep in mind we're not quite at the content level, we're testing the predictor which is in //chrome.
rdsmith@chromium.org changed reviewers: + juliatuttle@chromium.org
Charles: Sorry to keep bouncing this one at a high level, but I'm wondering if I'm the best reviewer. This seems like it's got a fair amount of DNS specific stuff in it, and while I'm coming up to speed on that from reviewing Julia's CLs, I'd think Julia would be the better reviewer. Julia: Do you have some familiarity with this code or interfaces? If so, could you do the review? If not, I'll keep it--I'm just trying to maximize overall efficiency. https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.h:336: friend class PredictorBrowserTest; Usually there's a way, if you friend the test fixture, to not have to friend the individual tests, by creating methods on the text fixture that reaches into the friended class. Did you look into doing that here? https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:260: // Note the rule based proc sleep synchronously. Sorry, I'm getting past the lexing phase, but failing to parse. What does this sentence mean?
On 2016/05/23 19:55:18, Randy Smith - Not in Fridays wrote: > Charles: Sorry to keep bouncing this one at a high level, but I'm wondering if > I'm the best reviewer. This seems like it's got a fair amount of DNS specific > stuff in it, and while I'm coming up to speed on that from reviewing Julia's > CLs, I'd think Julia would be the better reviewer. I'm fine with that. I mostly added you because you have some familiarity with the context, and Matt was overloaded. > > Julia: Do you have some familiarity with this code or interfaces? If so, could > you do the review? If not, I'll keep it--I'm just trying to maximize overall > efficiency. > > https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... > File chrome/browser/net/predictor.h (right): > > https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... > chrome/browser/net/predictor.h:336: friend class PredictorBrowserTest; > Usually there's a way, if you friend the test fixture, to not have to friend the > individual tests, by creating methods on the text fixture that reaches into the > friended class. Did you look into doing that here? > > https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... > File chrome/browser/net/predictor_browsertest.cc (right): > > https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... > chrome/browser/net/predictor_browsertest.cc:260: // Note the rule based proc > sleep synchronously. > Sorry, I'm getting past the lexing phase, but failing to parse. What does this > sentence mean?
rdsmith@chromium.org changed reviewers: - rdsmith@chromium.org
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
Removed myself from the review list in favor of Julia.
rdsmith@chromium.org changed reviewers: - rdsmith@chromium.org
Julia, PTAL. Randy, I'll add you back when I'm ready for a stamp (don't think Julia is in OWNERS). https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor.h:336: friend class PredictorBrowserTest; On 2016/05/23 19:55:18, Randy Smith - Not in Fridays wrote: > Usually there's a way, if you friend the test fixture, to not have to friend the > individual tests, by creating methods on the text fixture that reaches into the > friended class. Did you look into doing that here? Yeah I'll do that. https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1989363007/diff/40001/chrome/browser/net/pred... chrome/browser/net/predictor_browsertest.cc:260: // Note the rule based proc sleep synchronously. On 2016/05/23 19:55:18, Randy Smith - Not in Fridays wrote: > Sorry, I'm getting past the lexing phase, but failing to parse. What does this > sentence mean? Hahaha yeah that's extremely confusing. I meant the rule based host resolver proc *sleeps* synchronously, so AddToHistory call will be after the resolution is performed.
Description was changed from ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. BUG=610750,469120 ========== to ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. The patch also refactors how DNS browser tests work, where we query off the predictor's observer. BUG=610750,469120 ==========
csharrison@chromium.org changed reviewers: + rdsmith@chromium.org
+rdsmith because Julia is OOO. I've updated the CL to be a lot simpler. PTAL? The main change is that the predictor now notifies its observer (only attached for testing) of DNS lookup completions. This allows us to wait on the UI thread for lookups to complete without mucking with HostResolverProcs and posting tasks from the dns worker threads.
csharrison@chromium.org changed reviewers: + eroman@chromium.org - rdsmith@chromium.org
Actually, Randy is pretty swamped with reviews now too. Eric, would you mind taking a look?
not very familiar with the code, but LGTM https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:87: const char kChromiumHost[] = "http://chromium.org"; nit: Can you call this a "url" or "origin" now? https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:88: const char kInvalidLongHost[] = same https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:386: return successful_dns_lookups_.find(url) != successful_dns_lookups_.end() || optional: can use ContainsKey() to simplify the various x.find(y) != x.end() patterns. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:417: EXPECT_TRUE(HasHostBeenLookedUpLocked(url)) << "Expected to have looked up" nit: add a space at the end of the string. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:687: // This method verifies that |url| is in the predictors |results_| map. This nit: predictors --> predictor's https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:762: GURL goog("http://www.example.test/"); probably better to call this "url" -- not actually google. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:774: GURL goog("http://www.example.test"), goog2("http://gmail.google.com"), same here -- at least for one of them.
Thanks! https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:87: const char kChromiumHost[] = "http://chromium.org"; On 2016/05/27 00:56:38, eroman wrote: > nit: Can you call this a "url" or "origin" now? Done. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:88: const char kInvalidLongHost[] = On 2016/05/27 00:56:38, eroman wrote: > same Done. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:386: return successful_dns_lookups_.find(url) != successful_dns_lookups_.end() || On 2016/05/27 00:56:38, eroman wrote: > optional: can use ContainsKey() to simplify the various x.find(y) != x.end() > patterns. Done. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:417: EXPECT_TRUE(HasHostBeenLookedUpLocked(url)) << "Expected to have looked up" On 2016/05/27 00:56:38, eroman wrote: > nit: add a space at the end of the string. Done. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:687: // This method verifies that |url| is in the predictors |results_| map. This On 2016/05/27 00:56:38, eroman wrote: > nit: predictors --> predictor's Done. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:762: GURL goog("http://www.example.test/"); On 2016/05/27 00:56:38, eroman wrote: > probably better to call this "url" -- not actually google. Done. https://codereview.chromium.org/1989363007/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:774: GURL goog("http://www.example.test"), goog2("http://gmail.google.com"), On 2016/05/27 00:56:38, eroman wrote: > same here -- at least for one of them. Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1989363007/#ps120001 (title: "eroman@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989363007/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989363007/120001
Message was sent while issue was closed.
Description was changed from ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. The patch also refactors how DNS browser tests work, where we query off the predictor's observer. BUG=610750,469120 ========== to ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. The patch also refactors how DNS browser tests work, where we query off the predictor's observer. BUG=610750,469120 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. The patch also refactors how DNS browser tests work, where we query off the predictor's observer. BUG=610750,469120 ========== to ========== Move predictor dns unit tests to browser tests This patch moves some unit tests for the predictor to browser tests. This is in preparation for a move where the predictor will call out to //content for preresolve requests. The patch also refactors how DNS browser tests work, where we query off the predictor's observer. BUG=610750,469120 Committed: https://crrev.com/0597b217f6e2400796536e70ab3413743f4abf80 Cr-Commit-Position: refs/heads/master@{#396444} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0597b217f6e2400796536e70ab3413743f4abf80 Cr-Commit-Position: refs/heads/master@{#396444} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
