Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522303005:
Componentize builtin_provider_unittest (Closed)
DescriptionComponentize builtin_provider_unittest
BUG=569841
Patch Set 1 : #
Total comments: 6
Patch Set 2 : updated #
Total comments: 1
Messages
Total messages: 22 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Componentize builtin_provider_unittest BUG=569841 ========== to ========== Componentize builtin_provider_unittest BUG=569841 ==========
abhishek.a21@samsung.com changed reviewers: + droger@chromium.org
PTAL!
CC pkasting for a question: These tests seem to be a mix of unit tests and integration tests, since they make a lot of assumptions on the list of chrome:// URLs. I wonder if the right way to do proceed is the following: 1) keep the existing tests in chrome as is, in order not to lose the integration testing 2) make another version of the file in components that uses generic test strings instead of the actual chrome strings, and prune it from all the chrome-specific assumptions. What do you think? https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider_unittest.cc (right): https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:61: }; If we want to componentize this test, then it should not need all these chrome constants. Then you need to define some arrays of strings, like: const char* const kTestBuiltins[] = { "kBuiltinFoo", "kBuiltinBar", } const char* kTestUserTypeBuiltins[] { "kUserBuiltinFoo", "kUserBuiltinBar", } And change GetBuiltinURLs() and GetBuiltinsToProvideAsUserTypes() to just return these. Then you should update the tests to use these instead of the chrome:// URLs. This might be a bit tedious since the tests make very heavy assumptions about the values of the builtins (like how they share prefixes and such). You can get rid of all the android specific code. https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:71: : metrics::OmniboxInputType::INVALID; Is this really needed? Could we just always return metrics::OmniboxInputType::URL? https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:91: // std::vector<base::string16> builtins; Cleanup comments and debug code.
On 2016/01/08 15:00:05, droger wrote: > CC pkasting for a question: > These tests seem to be a mix of unit tests and integration tests, since they > make a lot of assumptions on the list of chrome:// URLs. > > I wonder if the right way to do proceed is the following: > 1) keep the existing tests in chrome as is, in order not to lose the integration > testing > 2) make another version of the file in components that uses generic test strings > instead of the actual chrome strings, and prune it from all the chrome-specific > assumptions. > > What do you think? It's not clear to me to what degree we actually do integration testing. If we were to remove all the chrome strings, what kind of coverage would we actually lose? I'm definitely keen on making the components file simply use generic strings, I just don't know if we need anything beyond that.
On 2016/01/09 01:13:46, Peter Kasting wrote: > On 2016/01/08 15:00:05, droger wrote: > > CC pkasting for a question: > > These tests seem to be a mix of unit tests and integration tests, since they > > make a lot of assumptions on the list of chrome:// URLs. > > > > I wonder if the right way to do proceed is the following: > > 1) keep the existing tests in chrome as is, in order not to lose the > integration > > testing > > 2) make another version of the file in components that uses generic test > strings > > instead of the actual chrome strings, and prune it from all the > chrome-specific > > assumptions. > > > > What do you think? > > It's not clear to me to what degree we actually do integration testing. If we > were to remove all the chrome strings, what kind of coverage would we actually > lose? > > I'm definitely keen on making the components file simply use generic strings, I > just don't know if we need anything beyond that. What seems clear though is that it's good to convert this test to use generic strings and move it to component. I'm not familiar with this test, so I didn't know if it was intended to be an integration test or not (for example it tests that typing "about://" in the omnibox suggests 3 specific URLs on desktop and 2 on Android). If this kind of things is not very valuable then we can just drop it. Maybe we can wait to see what the componentized test looks like and then see if it's worth adding the missing coverage back in chrome afterwards.
CC rohitrao: FYI, to make sure you're not working on the same unittest.
On 2016/01/11 09:47:15, droger wrote: > On 2016/01/09 01:13:46, Peter Kasting wrote: > > On 2016/01/08 15:00:05, droger wrote: > > > CC pkasting for a question: > > > These tests seem to be a mix of unit tests and integration tests, since they > > > make a lot of assumptions on the list of chrome:// URLs. > > > > > > I wonder if the right way to do proceed is the following: > > > 1) keep the existing tests in chrome as is, in order not to lose the > > integration > > > testing > > > 2) make another version of the file in components that uses generic test > > strings > > > instead of the actual chrome strings, and prune it from all the > > chrome-specific > > > assumptions. > > > > > > What do you think? > > > > It's not clear to me to what degree we actually do integration testing. If we > > were to remove all the chrome strings, what kind of coverage would we actually > > lose? > > > > I'm definitely keen on making the components file simply use generic strings, > I > > just don't know if we need anything beyond that. > > What seems clear though is that it's good to convert this test to use generic > strings and move it to component. > > I'm not familiar with this test, so I didn't know if it was intended to be an > integration test or not (for example it tests that typing "about://" in the > omnibox suggests 3 specific URLs on desktop and 2 on Android). > If this kind of things is not very valuable then we can just drop it. > Maybe we can wait to see what the componentized test looks like and then see if > it's worth adding the missing coverage back in chrome afterwards. +1
On 2016/01/11 09:47:15, droger wrote: > On 2016/01/09 01:13:46, Peter Kasting wrote: > > On 2016/01/08 15:00:05, droger wrote: > > > CC pkasting for a question: > > > These tests seem to be a mix of unit tests and integration tests, since they > > > make a lot of assumptions on the list of chrome:// URLs. > > > > > > I wonder if the right way to do proceed is the following: > > > 1) keep the existing tests in chrome as is, in order not to lose the > > integration > > > testing > > > 2) make another version of the file in components that uses generic test > > strings > > > instead of the actual chrome strings, and prune it from all the > > chrome-specific > > > assumptions. > > > > > > What do you think? > > > > It's not clear to me to what degree we actually do integration testing. If we > > were to remove all the chrome strings, what kind of coverage would we actually > > lose? > > > > I'm definitely keen on making the components file simply use generic strings, > I > > just don't know if we need anything beyond that. > > What seems clear though is that it's good to convert this test to use generic > strings and move it to component. > > I'm not familiar with this test, so I didn't know if it was intended to be an > integration test or not (for example it tests that typing "about://" in the > omnibox suggests 3 specific URLs on desktop and 2 on Android). > If this kind of things is not very valuable then we can just drop it. > Maybe we can wait to see what the componentized test looks like and then see if > it's worth adding the missing coverage back in chrome afterwards. I don't think the integration parts are necessary, so I'd go ahead with the route of "just remove Chrome-specific stuff and be done".
Patchset #2 (id:40001) has been deleted
PTAL! https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider_unittest.cc (right): https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:61: }; On 2016/01/08 15:00:05, droger wrote: > If we want to componentize this test, then it should not need all these chrome > constants. > > Then you need to define some arrays of strings, like: > > const char* const kTestBuiltins[] = { > "kBuiltinFoo", > "kBuiltinBar", > } > > const char* kTestUserTypeBuiltins[] { > "kUserBuiltinFoo", > "kUserBuiltinBar", > } > > And change GetBuiltinURLs() and GetBuiltinsToProvideAsUserTypes() to just return > these. > Thanks! I've made all the necessary changes. > Then you should update the tests to use these instead of the chrome:// URLs. > This might be a bit tedious since the tests make very heavy assumptions about > the values of the builtins (like how they share prefixes and such). > I'm checking this part, if any help, that will be a great push. > You can get rid of all the android specific code. Done. https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:71: : metrics::OmniboxInputType::INVALID; On 2016/01/08 15:00:05, droger wrote: > Is this really needed? Could we just always return > metrics::OmniboxInputType::URL? Done. https://codereview.chromium.org/1522303005/diff/20001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:91: // std::vector<base::string16> builtins; On 2016/01/08 15:00:05, droger wrote: > Cleanup comments and debug code. Done.
The CQ bit was checked by droger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522303005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522303005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The test does not pass. https://codereview.chromium.org/1522303005/diff/60001/chrome/browser/autocomp... File chrome/browser/autocomplete/builtin_provider_unittest.cc (right): https://codereview.chromium.org/1522303005/diff/60001/chrome/browser/autocomp... chrome/browser/autocomplete/builtin_provider_unittest.cc:37: const char kSyncSetupSubPage[] = "syncSetup"; We need to get rid of all of these and use the test constants you defined in kTestBuiltins and kTestUserTypeBuiltins instead.
blundell@chromium.org changed reviewers: + blundell@chromium.org
Hi Abhishek, are you going to be able to continue working on this or should one of us build on the work you've done so far to finish it off? Thanks!
On 2016/02/02 09:07:33, blundell wrote: > Hi Abhishek, are you going to be able to continue working on this or should one > of us build on the work you've done so far to finish it off? Thanks! Hi, Sorry, but currently I'm busy with other stuff! Feel free to take it up. Thanks!
On 2016/02/03 06:00:39, Abhishek wrote: > On 2016/02/02 09:07:33, blundell wrote: > > Hi Abhishek, are you going to be able to continue working on this or should > one > > of us build on the work you've done so far to finish it off? Thanks! > > Hi, > Sorry, but currently I'm busy with other stuff! > Feel free to take it up. > Thanks! Thanks! Rohit has https://codereview.chromium.org/1668543003/ going, which builds on the work you've done here. Thanks for that work, and you can close this one out :).
droger@chromium.org changed reviewers: - droger@chromium.org |
