Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
617103008:
Update chromoting browser-tests to obtain user-name and password from a specified file. (Closed)
DescriptionParse user-name and password values from a json file, instead of from command line.
BUG=419850
Committed: https://crrev.com/940c62c2aa3299f72d7195c77ae122a55dc78067
Cr-Commit-Position: refs/heads/master@{#298990}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Add checks for error conditions. Plus some style changes. #Patch Set 3 : Continue to get user-name and password from command line if no file has been specified. #
Total comments: 6
Patch Set 4 : Remove unnecessary member variables. Check for required values in Auth test. #
Total comments: 11
Patch Set 5 : Fixes for 2 different compile errors seen on Windows and Mac. #Patch Set 6 : Fix Windows-specific compile error. #
Messages
Total messages: 24 (8 generated)
anandc@chromium.org changed reviewers: + weitaosu@chromium.org
Patchset #1 (id:1) has been deleted
Hi Weitao, PTAL. Thanks.
On 2014/10/02 19:54:27, anandc wrote: > Hi Weitao, > > PTAL. Thanks. Boing. :-)
On 2014/10/06 15:18:48, anandc wrote: > On 2014/10/02 19:54:27, anandc wrote: > > Hi Weitao, > > > > PTAL. Thanks. > > Boing. :-) I still have some concerns about requiring a config file to run the browser test. The chrome browser tests, AFAIK, should be standalone and shouldn't require additional files. Let's discuss this in person. It will also help to consult with someone more familiar with chrome browser tests.
On 2014/10/06 16:56:24, weitaosu wrote: > On 2014/10/06 15:18:48, anandc wrote: > > On 2014/10/02 19:54:27, anandc wrote: > > > Hi Weitao, > > > > > > PTAL. Thanks. > > > > Boing. :-) > > I still have some concerns about requiring a config file to run the browser > test. The chrome browser tests, AFAIK, should be standalone and shouldn't > require additional files. Let's discuss this in person. It will also help to > consult with someone more familiar with chrome browser tests. Hi Weitao, Addressed some of the points you raised offline. PTAL at PS #3. Thank you.
https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.h (right): https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:24: const char kUserNameField[] = "username"; Isn't it called "user-name" in the json file? https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:372: std::string accounts_file_; Why store the file path if it's not used anywhere else? But if you do want to store it for use later I suggest that you store a scoped_ptr to the base::Value here rather than the file path. https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:373: std::string account_type_; Same as above. https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:818: void RemoteDesktopBrowserTest::SetUserNameAndPassword( I suggest that we don't store the accounts file path and account type as member variables. Rather just pass them in to SetUserNameAndPassword. https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:832: static_cast<base::DictionaryValue*>(root.get()); Please use GetAsDictionary. And instead of asserting the type is dictionary above, you can just assert the return result of GetAsDictionary. https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:835: root_dict->GetDictionary(account_type_, &account_dict); assert the result of GetDictionary.
Thank you Weitao. PTAL at PS#4 https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.h (right): https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:24: const char kUserNameField[] = "username"; On 2014/10/08 23:19:16, weitaosu wrote: > Isn't it called "user-name" in the json file? It was "user_name", but I then updated it to "username" to be consistent. Thanks. https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:372: std::string accounts_file_; On 2014/10/08 23:19:16, weitaosu wrote: > Why store the file path if it's not used anywhere else? But if you do want to > store it for use later I suggest that you store a scoped_ptr to the base::Value > here rather than the file path. Done. https://codereview.chromium.org/617103008/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:373: std::string account_type_; On 2014/10/08 23:19:16, weitaosu wrote: > Same as above. Done. https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:818: void RemoteDesktopBrowserTest::SetUserNameAndPassword( On 2014/10/08 23:19:16, weitaosu wrote: > I suggest that we don't store the accounts file path and account type as member > variables. Rather just pass them in to SetUserNameAndPassword. Done. https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:832: static_cast<base::DictionaryValue*>(root.get()); On 2014/10/08 23:19:16, weitaosu wrote: > Please use GetAsDictionary. And instead of asserting the type is dictionary > above, you can just assert the return result of GetAsDictionary. Done. https://codereview.chromium.org/617103008/diff/60001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:835: root_dict->GetDictionary(account_type_, &account_dict); On 2014/10/08 23:19:16, weitaosu wrote: > assert the result of GetDictionary. Done. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:479: ASSERT_TRUE(!username_.empty() && !password_.empty()); Added these just to fail quicker.
LGTM when all my comments are addressed. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:831: DCHECK(root.get() != NULL); Change to ASSERT_TRUE? https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:834: ASSERT_TRUE(root.get() && root->GetAsDictionary(&root_dict)); You have already DCHECKed on root.Get() earlier. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:839: Remove this blank line. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:841: ditto. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.h (right): https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:131: const base::FilePath &accounts_file, const std::string account_type); const std::string&
https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:831: DCHECK(root.get() != NULL); On 2014/10/09 00:23:28, weitaosu wrote: > Change to ASSERT_TRUE? Merged with the following ASSERT_TRUE. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:834: ASSERT_TRUE(root.get() && root->GetAsDictionary(&root_dict)); On 2014/10/09 00:23:28, weitaosu wrote: > You have already DCHECKed on root.Get() earlier. Done. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:839: On 2014/10/09 00:23:28, weitaosu wrote: > Remove this blank line. Done. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:841: On 2014/10/09 00:23:28, weitaosu wrote: > ditto. Done. https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.h (right): https://codereview.chromium.org/617103008/diff/80001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.h:131: const base::FilePath &accounts_file, const std::string account_type); On 2014/10/09 00:23:28, weitaosu wrote: > const std::string& Done.
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617103008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617103008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617103008/340001
Message was sent while issue was closed.
Committed patchset #6 (id:340001)
Message was sent while issue was closed.
Committed patchset #6 (id:340001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/940c62c2aa3299f72d7195c77ae122a55dc78067 Cr-Commit-Position: refs/heads/master@{#298990} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
