Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
153473004:
OOBE localization browser test (Closed)
|
Created:
6 years, 10 months ago by michaelpg Modified:
6 years, 10 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOOBE localization browser test
Test that the expected languages and keyboard layouts are available in the
OOBE network screen.
BUG=327034
R=nkostylev@chromium.org, achuith@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251227
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use selected configs #
Total comments: 26
Patch Set 3 : #Patch Set 4 : typo #Patch Set 5 : #
Total comments: 2
Patch Set 6 : test multiple locales #
Total comments: 12
Patch Set 7 : nits #
Total comments: 1
Messages
Total messages: 21 (0 generated)
This is a browser test to accomplish more or less the same thing as the autotest for OOBE localization. We can't check languages/keyboards that don't work in OOBE in our tests builds, e.g. the Russian keyboard. https://codereview.chromium.org/153473004/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:213: // To avoid timeout, run tests in sets of 5. Better ideas? There are like 55 languages and each one takes 3-4 seconds to test on my Z620.
+dzhioev@ Pavel, please help review this test since I'm out of office today.
On 2014/02/07 09:41:24, Nikita Kostylev wrote: > +dzhioev@ > > Pavel, please help review this test since I'm out of office today. -dzhioev@, +alemate@ Hi, I think Alexander understands how languages and layouts work much better than me.
https://codereview.chromium.org/153473004/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:62: void SetLocale(size_t index) { After https://codereview.chromium.org/144363006 initial_locale is now interpreted as a list of "recommended" locales. That also affect the way the list of locales is displayed at OOBE screen. After crbug.com/334576 this would also happen with keyboard_layout. It seems to me that testing all [theoretically] possible subsets of locales and keyboards will never work ;-) So let's select a few sets of locales and keyboards and test them. I would advise to three (strange) cases: 1) initial_locale="ru" initial_layout="xkb:ru::rus" 2) Any locale+keyboard requiring input methods extension. For example Japanese. Probably we should check all input methods extensions. 3) There is a test case (in cros_language_options_handler_unittest.cc ) for Icelandic language as an example of locale with input methods but no interface language. 4) Probably a variety of Swiss input methods should be a good example of unusual full-latin configuration. Please ask kenjibaheux@ for examples.
Test using a specific handful of cases - Russian, Japanese, Icelandic, French/Swiss and German/Swiss.
https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:30: class FakeStatisticsProvider : public StatisticsProvider { Do you mind not inlining the functions of this class (the ones that are not empty)? There's something in the style guide to this effect and there was a discussion on chromium-dev about how this is not desirable. I can dig up the link if you're interested. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:31: public: protected? https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:34: virtual void StartLoadingMachineStatistics( Shouldn't the rest of these be private? https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:39: // Populates the named machine statistic. Let's make this comment more verbose and say that we only populate initial_locale and keyboard_layout. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:74: namespace { newline after this I believe? I think it's better to have this anonymous namespace before the system namespace. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:78: const char* kUSLayout = "xkb:us::eng"; newline after this I believe? https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:81: class OobeLocalizationTest : public InProcessBrowserTest { Please de-inline this class too. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:89: void JsExpect(const std::string& expression) { I believe it's better for the caller of this function to do the actual assert (so you can see which function is erroring out without having to look at the stack). https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:158: system::FakeStatisticsProvider* statistics_provider() { This function seems unnecessary - why not just use statistics_provider_.get() instead? https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:163: scoped_ptr<system::FakeStatisticsProvider> statistics_provider_; Should this be StatisticsProvider instead of FakeStatisticsProvider? https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:171: RunLocalizationTest("ru", "xkb:ru::rus", If there's a danger of running into the time limit, you can break this up into 5 browser tests (or 2, or 3 by grouping a few tests). I would err on the side of making this as flake free as possible.
https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:30: class FakeStatisticsProvider : public StatisticsProvider { On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > Do you mind not inlining the functions of this class (the ones that are not > empty)? There's something in the style guide to this effect and there was a > discussion on chromium-dev about how this is not desirable. I can dig up the > link if you're interested. This is what I see in the majority of tests. There's some discussion about relaxing inline guidelines in .cc files, especially test files: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... and this thread: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/inlini... Did you read something more recently? I tried moving the function definitions out and it seems less readable to me. Given the discussions in those threads, do you still prefer to see this not inlined? https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:31: public: On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > protected? why? the browser test and the customization document classes need to access these functions. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:34: virtual void StartLoadingMachineStatistics( On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > Shouldn't the rest of these be private? The point of this fake StatisticsProvider is to be passed around and used -- this is outside of the OobeLocalizationTest class. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:39: // Populates the named machine statistic. On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > Let's make this comment more verbose and say that we only populate > initial_locale and keyboard_layout. Done. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:74: namespace { On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > newline after this I believe? I think it's better to have this anonymous > namespace before the system namespace. Done. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:78: const char* kUSLayout = "xkb:us::eng"; On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > newline after this I believe? Done. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:81: class OobeLocalizationTest : public InProcessBrowserTest { On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > Please de-inline this class too. Done. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:89: void JsExpect(const std::string& expression) { On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > I believe it's better for the caller of this function to do the actual assert > (so you can see which function is erroring out without having to look at the > stack). Done. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:158: system::FakeStatisticsProvider* statistics_provider() { On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > This function seems unnecessary - why not just use statistics_provider_.get() > instead? Done (this was vestigial, thanks for catching) https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:163: scoped_ptr<system::FakeStatisticsProvider> statistics_provider_; On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > Should this be StatisticsProvider instead of FakeStatisticsProvider? It can't be -- StatisticsProvider::~StatisticsProvider() is protected.
https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:30: class FakeStatisticsProvider : public StatisticsProvider { On 2014/02/10 23:38:26, Michael Giuffrida wrote: > On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > > Do you mind not inlining the functions of this class (the ones that are not > > empty)? There's something in the style guide to this effect and there was a > > discussion on chromium-dev about how this is not desirable. I can dig up the > > link if you're interested. > > This is what I see in the majority of tests. There's some discussion about > relaxing inline guidelines in .cc files, especially test files: > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > > and this thread: > https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/inli... > > Did you read something more recently? I tried moving the function definitions > out and it seems less readable to me. Given the discussions in those threads, do > you still prefer to see this not inlined? Yup, that's the thread. I guess it kind of went into the weeds after the first few posts, but I basically agree with PK that test code should be held to the same standards of normal code, and I also find long inlined function bodies make it difficult to figure out where the class begins and ends. I'll leave it up to you, but I prefer de-inlined, at least for longer functions. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:31: public: On 2014/02/10 23:38:26, Michael Giuffrida wrote: > On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > > protected? > > why? the browser test and the customization document classes need to access > these functions. I didn't realize the base class had a protected dtor. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:34: virtual void StartLoadingMachineStatistics( On 2014/02/10 23:38:26, Michael Giuffrida wrote: > On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > > Shouldn't the rest of these be private? > > The point of this fake StatisticsProvider is to be passed around and used -- > this is outside of the OobeLocalizationTest class. It's passed around and used via the base class pointer, which exposes these methods publicly. https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:163: scoped_ptr<system::FakeStatisticsProvider> statistics_provider_; On 2014/02/10 23:38:26, Michael Giuffrida wrote: > On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > > Should this be StatisticsProvider instead of FakeStatisticsProvider? > > It can't be -- StatisticsProvider::~StatisticsProvider() is protected. Ok
https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/350001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:34: virtual void StartLoadingMachineStatistics( Oh right, my mistake, private would work. It would break the Liskov substitution principle though -- is there a particular reason these methods should be private? On 2014/02/11 00:02:19, achuith.bhandarkar wrote: > On 2014/02/10 23:38:26, Michael Giuffrida wrote: > > On 2014/02/10 22:19:07, achuith.bhandarkar wrote: > > > Shouldn't the rest of these be private? > > > > The point of this fake StatisticsProvider is to be passed around and used -- > > this is outside of the OobeLocalizationTest class. > > It's passed around and used via the base class pointer, which exposes these > methods publicly.
https://codereview.chromium.org/153473004/diff/520001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/520001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:175: VerifySelectedOption(kLocaleSelect, expected_locale.c_str()); Could you please verify, that list of locales from initial_locale is displayed before other languages? There is a screenshot in crbug.com/342494. This is actually a <select> element: <select> <option>...</option> <option>...</option> <optgroup name "Other languages:"> ... </optgroup> </select> So we need to check that all first languages are from initial_locale.
https://codereview.chromium.org/153473004/diff/520001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/520001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:175: VerifySelectedOption(kLocaleSelect, expected_locale.c_str()); I've updated this function and added the NetworkScreenMultipleLocales test. On 2014/02/11 14:17:24, alemate wrote: > Could you please verify, that list of locales from initial_locale is displayed > before other languages? > > There is a screenshot in crbug.com/342494. > > This is actually a <select> element: > > <select> > <option>...</option> > <option>...</option> > <optgroup name "Other languages:"> > ... > </optgroup> > </select> > > So we need to check that all first languages are from initial_locale.
lgtm
lgtm with nits. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:54: virtual void StartLoadingMachineStatistics( nit: Please add a comment like: // StatisticsProvider overrides. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:129: "window.domAutomationController.send(!!(" + expression + "));", nit: could you pull this out into a separate variable? This function is a bit dense. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:136: std::string expression = base::StringPrintf( nit: const https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:156: std::string expression = base::StringPrintf( nit: const https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:178: // Initialize StartupCustomizationDocument with fake statistics provider nit: period at the end of the comment. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:182: // Bring up the OOBE network screen nit: period at the end of comment.
Nikita, would you mind stamping this for me? https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:54: virtual void StartLoadingMachineStatistics( On 2014/02/12 21:49:41, achuith.bhandarkar wrote: > nit: Please add a comment like: > // StatisticsProvider overrides. Done. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:129: "window.domAutomationController.send(!!(" + expression + "));", On 2014/02/12 21:49:41, achuith.bhandarkar wrote: > nit: could you pull this out into a separate variable? This function is a bit > dense. Done. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:136: std::string expression = base::StringPrintf( On 2014/02/12 21:49:41, achuith.bhandarkar wrote: > nit: const Done. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:156: std::string expression = base::StringPrintf( On 2014/02/12 21:49:41, achuith.bhandarkar wrote: > nit: const Done. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:178: // Initialize StartupCustomizationDocument with fake statistics provider On 2014/02/12 21:49:41, achuith.bhandarkar wrote: > nit: period at the end of the comment. Done. https://codereview.chromium.org/153473004/diff/580001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:182: // Bring up the OOBE network screen On 2014/02/12 21:49:41, achuith.bhandarkar wrote: > nit: period at the end of comment. Done.
lgtm https://codereview.chromium.org/153473004/diff/710001/chrome/browser/chromeos... File chrome/browser/chromeos/login/oobe_localization_browsertest.cc (right): https://codereview.chromium.org/153473004/diff/710001/chrome/browser/chromeos... chrome/browser/chromeos/login/oobe_localization_browsertest.cc:123: bool OobeLocalizationTest::JsExecute(const std::string& expression) { Much nicer - thanks!
lgtm
lgtm
lgtm
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/153473004/710001
Message was sent while issue was closed.
Change committed as 251227 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
