Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1237883002:
Added chromoting test environment for the tests to share data. (Closed)
DescriptionAdded chromoting test environment for the chromoting tests to share data.
I've refactored out the fake_refresh_token_store to a shared class so that both chromoting test environment and app remoting test environment can use it. I've also added a new class called fake_host_list_fetcher to fake retrieving a host list.
Added a new test and made some cleanups.
BUG=
Committed: https://crrev.com/ed3010ac30028a8126b9e80b2c6dcfd8b4d7b111
Cr-Commit-Position: refs/heads/master@{#340283}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Cleaned up environment code. #
Total comments: 22
Patch Set 3 : Clarified comments, removed unused headers, and fixed unit tests. #
Total comments: 6
Patch Set 4 : Merged and made changes to comments. #
Total comments: 8
Patch Set 5 : Fixed Merge Errors. #
Total comments: 6
Patch Set 6 : Fixed merge errors, comments, and BUILD.gn. #
Total comments: 8
Patch Set 7 : Added files to BUILD.gn and synced with connect to localhost changes. #
Total comments: 6
Patch Set 8 : Added unittest, display connection stats ability, and error check for transition delay. #Messages
Total messages: 36 (14 generated)
tonychun@google.com changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
Created chromoting test driver environment to share data between tests.
https://codereview.chromium.org/1237883002/diff/1/remoting/remoting_test.gypi File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:156: nit: remove extra newline https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:12: #include "base/at_exit.h" Not used, remove https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:15: #include "base/message_loop/message_loop.h" You can forward declare this, you don't need to include the header here https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:16: #include "remoting/test/access_token_fetcher.h" Forward declare, you only use pointers to AccessTokenFetchers so you don't need the header here. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:18: #include "remoting/test/host_list_fetcher.h" forward declare then remove https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:19: #include "remoting/test/refresh_token_store.h" Forward declare then remove https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:83: // |host_list_| is modified on failure. I think you can remove the second comment line, since this is a private function I don't think it is important to specify the implementation details of the class https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:121: // between tests and keeping long-lived objects around. Used to share auth s/auth/access https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment_unittest.cc:42: const ChromotingTestDriverEnvironment::EnvironmentOptions& options); The app remoting counterpart to this file uses two Initiaize() methods as I want to set different init params for some test cases, from the tests in this file I don't think you ever change the defaults so I would remove these two methods and instead implement the SetUp() interface method and add all of the initialization there, that way it ways magically for every test and you won't need to call initialize at the top of each test. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment_unittest.cc:106: // Attempt to init again, we should not see any additional calls or errors. Please update this string, it mentions 'any additional calls' which was appropriate when using the mock object as it would error if unexpected calls were made, but since you removed the mocks, it doesn't apply anymore, please fix for any other test cases that use it as well :) https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment_unittest.cc:106: // Attempt to init again, we should not see any additional calls or errors. Also, thanks for adding unit tests! :) https://codereview.chromium.org/1237883002/diff/1/remoting/test/fake_host_lis... File remoting/test/fake_host_list_fetcher.h (right): https://codereview.chromium.org/1237883002/diff/1/remoting/test/fake_host_lis... remoting/test/fake_host_list_fetcher.h:26: } I think there is supposed to be a space between the end of this function and 'private'. clang-format should fix that if so.
Cleaned up environment code. https://codereview.chromium.org/1237883002/diff/1/remoting/remoting_test.gypi File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/1/remoting/remoting_test.gypi... remoting/remoting_test.gypi:156: On 2015/07/15 03:04:53, joedow wrote: > nit: remove extra newline Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:12: #include "base/at_exit.h" On 2015/07/15 03:04:53, joedow wrote: > Not used, remove Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:15: #include "base/message_loop/message_loop.h" On 2015/07/15 03:04:53, joedow wrote: > You can forward declare this, you don't need to include the header here I used include because of what the C++ style guideline says to do. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... But now I see chromium does it differently: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:16: #include "remoting/test/access_token_fetcher.h" On 2015/07/15 03:04:53, joedow wrote: > Forward declare, you only use pointers to AccessTokenFetchers so you don't need > the header here. Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:18: #include "remoting/test/host_list_fetcher.h" On 2015/07/15 03:04:53, joedow wrote: > forward declare then remove Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:19: #include "remoting/test/refresh_token_store.h" On 2015/07/15 03:04:53, joedow wrote: > Forward declare then remove Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:83: // |host_list_| is modified on failure. On 2015/07/15 03:04:53, joedow wrote: > I think you can remove the second comment line, since this is a private function > I don't think it is important to specify the implementation details of the class Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment.h:121: // between tests and keeping long-lived objects around. Used to share auth On 2015/07/15 03:04:53, joedow wrote: > s/auth/access Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment_unittest.cc:42: const ChromotingTestDriverEnvironment::EnvironmentOptions& options); On 2015/07/15 03:04:53, joedow wrote: > The app remoting counterpart to this file uses two Initiaize() methods as I want > to set different init params for some test cases, from the tests in this file I > don't think you ever change the defaults so I would remove these two methods and > instead implement the SetUp() interface method and add all of the initialization > there, that way it ways magically for every test and you won't need to call > initialize at the top of each test. Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment_unittest.cc:106: // Attempt to init again, we should not see any additional calls or errors. On 2015/07/15 03:04:53, joedow wrote: > Also, thanks for adding unit tests! :) Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/chromoting_te... remoting/test/chromoting_test_driver_environment_unittest.cc:106: // Attempt to init again, we should not see any additional calls or errors. On 2015/07/15 03:04:53, joedow wrote: > Please update this string, it mentions 'any additional calls' which was > appropriate when using the mock object as it would error if unexpected calls > were made, but since you removed the mocks, it doesn't apply anymore, please fix > for any other test cases that use it as well :) Done. https://codereview.chromium.org/1237883002/diff/1/remoting/test/fake_host_lis... File remoting/test/fake_host_list_fetcher.h (right): https://codereview.chromium.org/1237883002/diff/1/remoting/test/fake_host_lis... remoting/test/fake_host_list_fetcher.h:26: } On 2015/07/15 03:04:53, joedow wrote: > I think there is supposed to be a space between the end of this function and > 'private'. clang-format should fix that if so. Done.
anandc@chromium.org changed reviewers: + anandc@chromium.org
https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.h:95: // Used to find remote host in host lost. lost=list https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.h:99: std::string user_name_; Change to: "The test-account for a test case". Yes, it is also used for authentication, but that's primarily because we want to test with it. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.h:104: // Host info in host list used to connect to remote host. ? Did you mean: "List of remote-hosts for the specified user/test-account" ?
https://codereview.chromium.org/1237883002/diff/40001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/remoting_test.... remoting/remoting_test.gypi:59: 'test/chromoting_test_driver_environment.h', You will want to coordinate the landing of this change with Sergey as he is making some changes in this file as well. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.cc:143: // still valid. If you update your chromoting_test_driver main() function with an AtExitManager like I did for the ARTD, then you won't have to worry about this problem. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.cc:259: VLOG(1) << "Host List Size: " << host_list_.size(); Is the size important to log vs. the actual hosts retrieved (i.e. if you retrieved 15 hosts but none of them matched the input param it wouldn't really matter)? I'd assume that the user would pass a flag to display all available hosts which would be more useful. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:7: #include <algorithm> I used algorithm for sorting the list of hosts to release, I didn't see it used in this file though, remove this line if it isn't needed. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:11: #include "remoting/test/fake_app_remoting_report_issue_request.h" Pretty sure you don't need this file...please remove https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:14: #include "remoting/test/refresh_token_store.h" I don't think this header is needed since you only use the FakeRefreshTokenStore https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:87: EXPECT_TRUE(environment_object_->Initialize(kAuthCodeValue)); some newlines here would make this much more readable. Insert one here and another between the fake_token_store checks and the environment_object checks https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:93: EXPECT_FALSE(environment_object_->host_list().empty()); Should be validating that the data in the host_list matches what you set previously?
Finished touching up the comments and removed the unnecessary header files from my unit test code. I also added host_list_fetcher into BUILD.gn so that the try bots can all succeed. https://codereview.chromium.org/1237883002/diff/40001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/remoting_test.... remoting/remoting_test.gypi:59: 'test/chromoting_test_driver_environment.h', On 2015/07/16 03:00:38, joedow wrote: > You will want to coordinate the landing of this change with Sergey as he is > making some changes in this file as well. Talked to him. I will merge with his changes if his lands first. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.cc:143: // still valid. On 2015/07/16 03:00:38, joedow wrote: > If you update your chromoting_test_driver main() function with an AtExitManager > like I did for the ARTD, then you won't have to worry about this problem. Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.cc:259: VLOG(1) << "Host List Size: " << host_list_.size(); On 2015/07/16 03:00:38, joedow wrote: > Is the size important to log vs. the actual hosts retrieved (i.e. if you > retrieved 15 hosts but none of them matched the input param it wouldn't really > matter)? I'd assume that the user would pass a flag to display all available > hosts which would be more useful. Removed. I agree with your point. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.h:95: // Used to find remote host in host lost. On 2015/07/15 23:51:58, anandc wrote: > lost=list Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.h:99: std::string user_name_; On 2015/07/15 23:51:58, anandc wrote: > Change to: "The test-account for a test case". > Yes, it is also used for authentication, but that's primarily because we want to > test with it. Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.h:104: // Host info in host list used to connect to remote host. On 2015/07/15 23:51:58, anandc wrote: > ? Did you mean: "List of remote-hosts for the specified user/test-account" ? Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:7: #include <algorithm> On 2015/07/16 03:00:38, joedow wrote: > I used algorithm for sorting the list of hosts to release, I didn't see it used > in this file though, remove this line if it isn't needed. Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:11: #include "remoting/test/fake_app_remoting_report_issue_request.h" On 2015/07/16 03:00:38, joedow wrote: > Pretty sure you don't need this file...please remove Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:14: #include "remoting/test/refresh_token_store.h" On 2015/07/16 03:00:38, joedow wrote: > I don't think this header is needed since you only use the FakeRefreshTokenStore Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:87: EXPECT_TRUE(environment_object_->Initialize(kAuthCodeValue)); On 2015/07/16 03:00:39, joedow wrote: > some newlines here would make this much more readable. Insert one here and > another between the fake_token_store checks and the environment_object checks Done. https://codereview.chromium.org/1237883002/diff/40001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:93: EXPECT_FALSE(environment_object_->host_list().empty()); On 2015/07/16 03:00:39, joedow wrote: > Should be validating that the data in the host_list matches what you set > previously? Done.
https://codereview.chromium.org/1237883002/diff/80001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/80001/remoting/remoting_test.... remoting/remoting_test.gypi:59: 'test/chromoting_test_driver_environment.h', please sync sergey's changes in and include in the next iteration. https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.cc:139: message_loop_.reset(); The comment was still useful (I think it should be added back), I was commenting that you can remove this logic once you add the AtExitManager code in your main file. Since that change isn't included in this CL, then you should leave the comment and this logic in place. https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:84: // Pass in an empty auth code since we are using a refresh token. Copy paste error? You aren't passing in an empty auth code, you are passing in a constant value.
Patchset #6 (id:100001) has been deleted
Merged and added comments. https://codereview.chromium.org/1237883002/diff/80001/remoting/remoting_test.... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/80001/remoting/remoting_test.... remoting/remoting_test.gypi:59: 'test/chromoting_test_driver_environment.h', On 2015/07/16 21:32:58, joedow wrote: > please sync sergey's changes in and include in the next iteration. Done. https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment.cc:139: message_loop_.reset(); On 2015/07/16 21:32:58, joedow wrote: > The comment was still useful (I think it should be added back), I was commenting > that you can remove this logic once you add the AtExitManager code in your main > file. Since that change isn't included in this CL, then you should leave the > comment and this logic in place. I see. Got it https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/80001/remoting/test/chromotin... remoting/test/chromoting_test_driver_environment_unittest.cc:84: // Pass in an empty auth code since we are using a refresh token. On 2015/07/16 21:32:58, joedow wrote: > Copy paste error? You aren't passing in an empty auth code, you are passing in > a constant value. Done.
https://codereview.chromium.org/1237883002/diff/120001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1237883002/diff/120001/remoting/BUILD.gn#newc... remoting/BUILD.gn:78: sources = [ Sergey had removed these files and moved them into the remoting/test:test_support target, I think you accidentally merged them back here. https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... remoting/remoting_test.gypi:60: 'test/app_remoting_test_driver_environment_app_details.cc', Does the ar_test_driver target still build with this app details included here? https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... remoting/remoting_test.gypi:132: 'test/host_list_fetcher.h', I don't think you need these since they are already included in remoting_test_support https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... remoting/remoting_test.gypi:342: 'test/app_remoting_test_driver_environment_unittest.cc', This file is already included (line 359)
Fixed merge errors. https://codereview.chromium.org/1237883002/diff/120001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1237883002/diff/120001/remoting/BUILD.gn#newc... remoting/BUILD.gn:78: sources = [ On 2015/07/16 23:15:38, joedow wrote: > Sergey had removed these files and moved them into the > remoting/test:test_support target, I think you accidentally merged them back > here. Shoot! Will remove. https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... remoting/remoting_test.gypi:60: 'test/app_remoting_test_driver_environment_app_details.cc', On 2015/07/16 23:15:38, joedow wrote: > Does the ar_test_driver target still build with this app details included here? Yes it does. https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... remoting/remoting_test.gypi:132: 'test/host_list_fetcher.h', On 2015/07/16 23:15:38, joedow wrote: > I don't think you need these since they are already included in > remoting_test_support Removed. https://codereview.chromium.org/1237883002/diff/120001/remoting/remoting_test... remoting/remoting_test.gypi:342: 'test/app_remoting_test_driver_environment_unittest.cc', On 2015/07/16 23:15:38, joedow wrote: > This file is already included (line 359) Removed.
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Fixed merge errors.
Missed one repeated test case.
Missed one repeated test case.
Patchset #7 (id:220001) has been deleted
lgtm https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.cc:7: #include <map> Are you using a map here? Pretty sure this can be removed. https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:98: // The test-account for a test case. nit: why the hyphen in test-account? https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:104: // List of remote-hosts for the specified user/test-account. nit: why a hyphen in remote-hosts?
Patchset #8 (id:260001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Fixed merge errors, comments, and BUILD.gn. https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.cc:7: #include <map> On 2015/07/17 02:35:49, joedow wrote: > Are you using a map here? Pretty sure this can be removed. Removed. https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:98: // The test-account for a test case. On 2015/07/17 02:35:49, joedow wrote: > nit: why the hyphen in test-account? Done. https://codereview.chromium.org/1237883002/diff/240001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:104: // List of remote-hosts for the specified user/test-account. On 2015/07/17 02:35:49, joedow wrote: > nit: why a hyphen in remote-hosts? Done.
https://codereview.chromium.org/1237883002/diff/280001/remoting/test/BUILD.gn File remoting/test/BUILD.gn (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/BUILD.gn... remoting/test/BUILD.gn:26: "fake_refresh_token_store.h", Please add here all other files that you are adding in this CL, e.g. fake_host_list_fetcher.cc https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.cc:23: ChromotingTestDriverEnvironment* ChromotingSharedData; I don't see this being used in this CL? Can it be avoided? In either case this is not a proper name for a global variable. See https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names Also conventional way to make singletons is to have this variable hidden and with a static function ChromotingTestDriverEnvironment::Instance() to access it. https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:125: extern ChromotingTestDriverEnvironment* ChromotingSharedData; I don't see this used anywhere. In either case see my comments in the .cc file https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment_unittest.cc:77: ChromotingTestDriverEnvironmentTest::ChromotingTestDriverEnvironmentTest() { nit: Move constructor and destructor before SetUp()
Merged with changes to connect to localhost and removed global pointer. I will use Sergey's suggestion for a singleton to access the environment variables in a future CL. https://codereview.chromium.org/1237883002/diff/280001/remoting/test/BUILD.gn File remoting/test/BUILD.gn (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/BUILD.gn... remoting/test/BUILD.gn:26: "fake_refresh_token_store.h", On 2015/07/20 19:37:13, Sergey Ulanov wrote: > Please add here all other files that you are adding in this CL, e.g. > fake_host_list_fetcher.cc Done. https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.cc:23: ChromotingTestDriverEnvironment* ChromotingSharedData; On 2015/07/20 19:37:13, Sergey Ulanov wrote: > I don't see this being used in this CL? Can it be avoided? > In either case this is not a proper name for a global variable. See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names > > Also conventional way to make singletons is to have this variable hidden and > with a static function ChromotingTestDriverEnvironment::Instance() to access it. I will remove it now and add it in once I add in the tests to use it. https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:125: extern ChromotingTestDriverEnvironment* ChromotingSharedData; On 2015/07/20 19:37:13, Sergey Ulanov wrote: > I don't see this used anywhere. In either case see my comments in the .cc file Removed it. https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/1237883002/diff/280001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment_unittest.cc:77: ChromotingTestDriverEnvironmentTest::ChromotingTestDriverEnvironmentTest() { On 2015/07/20 19:37:13, Sergey Ulanov wrote: > nit: Move constructor and destructor before SetUp() Done.
lgtm after my comments are addressed https://codereview.chromium.org/1237883002/diff/300001/remoting/remoting_test... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/300001/remoting/remoting_test... remoting/remoting_test.gypi:330: 'test/chromoting_test_driver_environment_unittest.cc', please add this file in BUILD.gn https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.cc:24: : refresh_token_file_path(base::FilePath()) { Don't need this initializer. The default constructor will be called automatically anyway. https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:125: extern ChromotingTestDriverEnvironment* ChromotingSharedData; Remove this since you've removed it from .cc file.
Patchset #8 (id:320001) has been deleted
The CQ bit was checked by tonychun@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1237883002/#ps340001 (title: "Added unittest, display connection stats ability, and error check for transition delay.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1237883002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1237883002/340001
https://codereview.chromium.org/1237883002/diff/300001/remoting/remoting_test... File remoting/remoting_test.gypi (right): https://codereview.chromium.org/1237883002/diff/300001/remoting/remoting_test... remoting/remoting_test.gypi:330: 'test/chromoting_test_driver_environment_unittest.cc', On 2015/07/23 19:12:43, Sergey Ulanov wrote: > please add this file in BUILD.gn Done. https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.cc (right): https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.cc:24: : refresh_token_file_path(base::FilePath()) { On 2015/07/23 19:12:43, Sergey Ulanov wrote: > Don't need this initializer. The default constructor will be called > automatically anyway. Done. https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... File remoting/test/chromoting_test_driver_environment.h (right): https://codereview.chromium.org/1237883002/diff/300001/remoting/test/chromoti... remoting/test/chromoting_test_driver_environment.h:125: extern ChromotingTestDriverEnvironment* ChromotingSharedData; On 2015/07/23 19:12:43, Sergey Ulanov wrote: > Remove this since you've removed it from .cc file. Done.
Message was sent while issue was closed.
Committed patchset #8 (id:340001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ed3010ac30028a8126b9e80b2c6dcfd8b4d7b111 Cr-Commit-Position: refs/heads/master@{#340283} |
