Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
344083003:
Stop using UIThreadSearchTermsData in tests when unnecessary (Closed)
|
Created:
6 years, 6 months ago by hashimoto Modified:
6 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Base URL:
http://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionStop using UIThreadSearchTermsData in tests when unnecessary
Implement fake methods in TestSearchTermsData to test TemplateURL's code without relying on UIThreadSearchTermsData's behavior.
BUG=371535
TEST=unit_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278625
Patch Set 1 : #
Total comments: 4
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:716: base::string16 rlz_string = search_terms_data_.GetRlzParameterValue(false); These next two functions no longer check that the real RLZ string gets subbed in. Was that testing important?
https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:716: base::string16 rlz_string = search_terms_data_.GetRlzParameterValue(false); On 2014/06/19 21:24:42, Peter Kasting wrote: > These next two functions no longer check that the real RLZ string gets subbed > in. Was that testing important? IIUC the purpose of these tests are to verify that "{google:RLZ}" in the template and |from_app_list| of SearchTermsArgs are correctly handled by TemplateURL. Even in the new code, these tests are serving for that purpose.
(LGTM if the below point is addressed) https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:716: base::string16 rlz_string = search_terms_data_.GetRlzParameterValue(false); On 2014/06/19 21:43:50, hashimoto wrote: > On 2014/06/19 21:24:42, Peter Kasting wrote: > > These next two functions no longer check that the real RLZ string gets subbed > > in. Was that testing important? > > IIUC the purpose of these tests are to verify that "{google:RLZ}" in the > template and |from_app_list| of SearchTermsArgs are correctly handled by > TemplateURL. > Even in the new code, these tests are serving for that purpose. Right, I am concerned that the tests may be doing double duty as both what you said, and actually verifying the RLZ code in sort of an end-to-end fashion. You should probably ensure that there are separate tests of the RLZ code elsewhere and/or that an RLZ owner signs off on this.
https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... File chrome/browser/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... chrome/browser/search_engines/template_url_unittest.cc:716: base::string16 rlz_string = search_terms_data_.GetRlzParameterValue(false); On 2014/06/19 22:08:36, Peter Kasting wrote: > On 2014/06/19 21:43:50, hashimoto wrote: > > On 2014/06/19 21:24:42, Peter Kasting wrote: > > > These next two functions no longer check that the real RLZ string gets > subbed > > > in. Was that testing important? > > > > IIUC the purpose of these tests are to verify that "{google:RLZ}" in the > > template and |from_app_list| of SearchTermsArgs are correctly handled by > > TemplateURL. > > Even in the new code, these tests are serving for that purpose. > > Right, I am concerned that the tests may be doing double duty as both what you > said, and actually verifying the RLZ code in sort of an end-to-end fashion. > > You should probably ensure that there are separate tests of the RLZ code > elsewhere and/or that an RLZ owner signs off on this. chrome/browser/rlz/rlz_unittest.cc is testing RLZTracker::GetAccessPointRlz() and other RLZ functions. Do you think this is sufficient?
On 2014/06/19 22:37:10, hashimoto wrote: > https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... > File chrome/browser/search_engines/template_url_unittest.cc (right): > > https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... > chrome/browser/search_engines/template_url_unittest.cc:716: base::string16 > rlz_string = search_terms_data_.GetRlzParameterValue(false); > On 2014/06/19 22:08:36, Peter Kasting wrote: > > On 2014/06/19 21:43:50, hashimoto wrote: > > > On 2014/06/19 21:24:42, Peter Kasting wrote: > > > > These next two functions no longer check that the real RLZ string gets > > subbed > > > > in. Was that testing important? > > > > > > IIUC the purpose of these tests are to verify that "{google:RLZ}" in the > > > template and |from_app_list| of SearchTermsArgs are correctly handled by > > > TemplateURL. > > > Even in the new code, these tests are serving for that purpose. > > > > Right, I am concerned that the tests may be doing double duty as both what you > > said, and actually verifying the RLZ code in sort of an end-to-end fashion. > > > > You should probably ensure that there are separate tests of the RLZ code > > elsewhere and/or that an RLZ owner signs off on this. > > chrome/browser/rlz/rlz_unittest.cc is testing RLZTracker::GetAccessPointRlz() > and other RLZ functions. > Do you think this is sufficient? I don't know, because I know nothing about RLZ. I'm leaving it to you to make the call or find whom to ask if necessary.
On 2014/06/19 22:39:07, Peter Kasting wrote: > On 2014/06/19 22:37:10, hashimoto wrote: > > > https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... > > File chrome/browser/search_engines/template_url_unittest.cc (right): > > > > > https://codereview.chromium.org/344083003/diff/20001/chrome/browser/search_en... > > chrome/browser/search_engines/template_url_unittest.cc:716: base::string16 > > rlz_string = search_terms_data_.GetRlzParameterValue(false); > > On 2014/06/19 22:08:36, Peter Kasting wrote: > > > On 2014/06/19 21:43:50, hashimoto wrote: > > > > On 2014/06/19 21:24:42, Peter Kasting wrote: > > > > > These next two functions no longer check that the real RLZ string gets > > > subbed > > > > > in. Was that testing important? > > > > > > > > IIUC the purpose of these tests are to verify that "{google:RLZ}" in the > > > > template and |from_app_list| of SearchTermsArgs are correctly handled by > > > > TemplateURL. > > > > Even in the new code, these tests are serving for that purpose. > > > > > > Right, I am concerned that the tests may be doing double duty as both what > you > > > said, and actually verifying the RLZ code in sort of an end-to-end fashion. > > > > > > You should probably ensure that there are separate tests of the RLZ code > > > elsewhere and/or that an RLZ owner signs off on this. > > > > chrome/browser/rlz/rlz_unittest.cc is testing RLZTracker::GetAccessPointRlz() > > and other RLZ functions. > > Do you think this is sufficient? > > I don't know, because I know nothing about RLZ. I'm leaving it to you to make > the call or find whom to ask if necessary. OK, what the existing test is doing is: 1. Constructing the expected RLZ string with its own copy of UIThreadSearchTermsData::GetRlzParameterValue() (calling RLZTracker::GetAccessPointRlz() after checking the brand). 2. With the expected RLZ string, verifying that TemplateURL is correctly handling "{google:RLZ}" and |from_app_list|. The new code is still doing #2. Regarding #1, since RLZTracker::GetAccessPointRlz() is tested well in rlz_unittest.cc, there is almost no point for doing it. It's almost like doing: base::strin16 rlz_string = UIThreadSearchMetadata(NULL).GetRlzParameterValue(); EXPECT_EQ(rlz_string, rlz_string); The only benefit we can get from doing this is that we can verify UIThreadSearchMetadata::GetRlzParameterValue() doesn't crash during the test. So I think it's OK to switch to the new code. With the new code, we can test that "{google:RLZ}" is correctly handled even with trybots which are running RLZ disabled binaries. If we really want to have a test which goes through UIThreadSearchMetadata::GetRlzParameterValue(), we can add ui_thread_search_metadata_unittest.cc for doing it. Does it make sense?
Sounds fine.
The CQ bit was checked by hashimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hashimoto@chromium.org/344083003/20001
Message was sent while issue was closed.
Change committed as 278625 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
