Adding Test Fixture for initial test cases for the App Remoting Test Driver. Also includes the pub…

Adding Test Fixture for initial test cases for the App Remoting Test Driver.  Also includes the public stubs for the remote application data methods.

Also ran clang-format on edited files to keep consistent with formatting
in my previous changelist.

BUG=467200
Committed: https://crrev.com/2230e1f901c84ce7fb9a69b2055ba9879a0461a2
Cr-Commit-Position: refs/heads/master@{#322119}

[joedow, 2015-03-13 23:27:45]

joedow@chromium.org changed reviewers:
+ wez@chromium.org

[joedow, 2015-03-13 23:27:46]

This is the last CL for the App Remoting Test Driver <whew>.  It introduces a
Test Fixture that handles connecting the client to a remote host (and which will
be used by test cases later on).  I've also added some code which will show the
status of the known remote hosts (which is useful for diagnostic purposes).

[Wez, 2015-03-16 18:26:35]

On 2015/03/13 23:27:46, joedow wrote:
> This is the last CL for the App Remoting Test Driver <whew>.  It introduces a
> Test Fixture that handles connecting the client to a remote host (and which
will
> be used by test cases later on).  I've also added some code which will show
the
> status of the known remote hosts (which is useful for diagnostic purposes).

Just getting to looking at this. Is "Test Fixture" a class in here - don't see a
file with that name? If not, then should it be capitalized in the description?

[chromium-reviews, 2015-03-16 18:29:35]

Starting with a comment on the CL description?  This is going to be fun ;)

The suggested way to name test fixtures is to use the 'Tests' suffix so
the app_remoting_connected_client_tests.* files represent the fixture.

Thanks,
Joe

On Mon, Mar 16, 2015 at 11:26 AM, <wez@chromium.org> wrote:

> On 2015/03/13 23:27:46, joedow wrote:
>
>> This is the last CL for the App Remoting Test Driver <whew>.  It
>> introduces a
>> Test Fixture that handles connecting the client to a remote host (and
>> which
>>
> will
>
>> be used by test cases later on).  I've also added some code which will
>> show
>>
> the
>
>> status of the known remote hosts (which is useful for diagnostic
>> purposes).
>>
>
> Just getting to looking at this. Is "Test Fixture" a class in here - don't
> see a
> file with that name? If not, then should it be capitalized in the
> description?
>
> https://codereview.chromium.org/1008043003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2015-03-16 22:19:10]

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_tests.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:35: }
nit: This is a non-trivial namespace declaration, so put a // namespace comment
at the end.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:41:
AppRemotingConnectedClientTests::message_loop_;
Does this line really need wrapping?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:54: // before each test).
Which of these properties is the important one to note?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:55: DCHECK(client_);
No point DCHECKing a pointer immediately before dereferencing it!

In DEBUG builds scoped_ptr<> will actually DCHECK for you, in fact.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:75: DCHECK(client_);
See above re DCHECK

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:93: // members are
destroyed via DeleteSoon on the message loop's TaskRunner.
Unless you actually run the message-loop after destroying the client, though, it
won't execute any tasks that are posted to it.

If you want that to happen then I'd suggest adding:

base::RunLoop().RunUntilIdle();

between the |client_| and |message_loop_| reset() calls.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:106: done_closure_ =
run_loop.QuitClosure();
Why are you saving the QuitClosure() for a RunLoop you've allocated on the stack
into an object member variable?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:112: max_wait_time);
Cleaner to use a base::Timer for this?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:139: done_closure_ =
run_loop.QuitClosure();
See above, re why this needs saving.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:151: wait_time);
See above re base::Timer

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:161: void
AppRemotingConnectedClientTests::ConnectionStateChanged(
The RemoteConnectionObserver APIs are called by the |client_| - what thread will
that use? Consider DCHECKing that the thread is the same as the one on which the
test-case was instantiated.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:172: error_code !=
protocol::OK)) {
Why are these two connection-state checks different? e.g. When would we ever
want to transition into the ready-for-tests=false state without quitting the
test run-loop?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:177: void
AppRemotingConnectedClientTests::ConnectionReady(bool ready) {
Under what circumstances will we receive this call with |ready| false? Should we
be running |done_callback_| in those cases?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:185: if
(!host_message_received_callback_.is_null()) {
Do we really want to ignore host messages received while we're not expecting
them?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:199: // next send the host
a description of the client screen size.
nit: Capitalization

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:214: 
nit: No need for this blank line.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:216:
video_control.set_lossless_color(true);
By default these are currently off (false).

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:223: const std::string&
main_window_name,
It seems strange to pass main_window_name in as a bound parameter, even though
this handler function is a member of the class, so has access to members of the
test instance object itself.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:230: LOG(ERROR) << "Message
received was not valid JSON.";
Should we terminate the test (e.g. via |done_callback_| in this case?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:239: << "underlying
application is in a bad state, YMMV.";
I'm surprised that this is ever a state we get into! Is this something you see
in real test runs?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:250: FROM_HERE,
done_closure_, wait_time);
Consider replacing this with a base::Timer member of the test object.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:252: }
Do we want to completely ignore all other window messages? Or at least verify
that they are one of the ones we expect / know about, and fail otherwise?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_tests.h (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:30: // Creates a connection
to a remote host when the static SetUpTestCase() method
This comment should describe the overall intended purpose of this fixture (i.e.
to set up common test state such as the connection to the host) and then
describe the things it does to achieve that, if you need to; as it is it is very
implementation-centric!

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:32: // This means that tests
which use this fixture can rely on having a connection
"This means that" is superfluous.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:35: // name of the
application we want to test.
This sentence is pretty confusingly worded!

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:36: class
AppRemotingConnectedClientTests
Why is this class "*Tests" rather than "*Test"? Each derived instance is a
single test, isn't it, not multiple tests?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:43: // static testing::Test
overrides.
Since these are static, they are presumably overrides only for this
instantiation of the  testing::TestWithParam template?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:71: // Starts a connection
with the remote host defined by the parameter..
typo: spurious .

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:79: // for the title is
passed in.
This comment is confusing since it doesn't relate anything back to |message|
directly, and its name doesn't seem to fit its function, going by the comment -
it seems that this method actually calls |done_closure_| if the contents of
|message| match |main_window_name|, in which case shouldn't this be
CallDoneCallbackIfWindowMatches() or similar?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:92:
HostMessageReceivedCallback host_message_received_callback_;
It's not clear how tests can customize this, since it's a private member and
there are no protected nor public APIs to set it?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:103: static
scoped_ptr<TestChromotingClient> client_;
Do we really want to re-use a connection across tests?

That means tests may acquire implicit dependencies upon one another and break if
they are re-ordered, for example.

Is the test framework single-threaded? Or might it run multiple fixtures on
different threads in the same process, at the same time?

Also note that we don't typically use scoped_ptr<> in statics in Chromium code,
since non-POD global variables cause the compiler to spit out initializer and
finalizer code, and that makes process startup & shutdown slower, amongst other
things. It can also lead to issues if any global statics end up depending on, or
are depended on, by other classes.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.cc:22: const char
kHostAvailabilityFormatString[] = "%-25s%-40s%-10s\n";
You can define this immediately before the printf where you use it (and then you
don't need an anonymous namespace (which you didn't strictly need anyway, since
it's const :))

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.cc:164:
info.application_id.c_str(), status.c_str());
Is it normal to use printf() in test fixtures, rather than LOG() etc?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.h (left):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.h:68: // |remote_host_info|
is not modified on failure.
RemoteHostInfo _is_ now modified on failure, then?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.h (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.h:43: // their availability.
Displays it where?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
File remoting/test/remote_application_data.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.cc:17: return
remoting::test::kRemoteApplicationNames;
Why do you want to return an empty vector here..?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.cc:23: return
remoting::test::kRemoteApplicationInfoMap;
Why do you want to return an empty map here...?

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
File remoting/test/remote_application_data.h (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.h:6: #define
REMOTING_TEST_REMOTE_APPLICATION_DATA_H_
nit: Make the filename & guard macro consistent with the struct name, below.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.h:28: const std::vector<std::string>&
GetRemoteApplicationNames();
It's confusing that you refer to this as the "application name", "window name"
or "main window name" in different places.

As a result it's not clear whether this function returns actual application
names, application Ids, the names of all application windows, or the names only
of each application's "main" window.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.h:31: const std::map<std::string,
ApplicationInfo>& GetRemoteApplicationInfoMap();
Again, it's not clear what the "name" is in this index, versus Id, window name,
etc.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_host_i...
File remoting/test/remote_host_info_fetcher.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_host_i...
remoting/test/remote_host_info_fetcher.cc:73:
request_->SetRequestContext(request_context_getter_.get());
SetRequestContext takes a reference to the getter (the API should really be
updated to take a const scoped_refptr<>&...) , so you can just pass it in
without keeping a reference to it yourself.

[joedow, 2015-03-18 20:13:10]

Addressed feedback from iteration #1, PTAL!

Thanks,
Joe

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_tests.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:35: }
On 2015/03/16 22:19:08, Wez wrote:
> nit: This is a non-trivial namespace declaration, so put a // namespace
comment
> at the end.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:41:
AppRemotingConnectedClientTests::message_loop_;
On 2015/03/16 22:19:08, Wez wrote:
> Does this line really need wrapping?

It does :(

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:54: // before each test).
On 2015/03/16 22:19:09, Wez wrote:
> Which of these properties is the important one to note?

Done.  Removed since I got rid of the static methods so there shouldn't be any
confusion as to when these run now.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:55: DCHECK(client_);
On 2015/03/16 22:19:09, Wez wrote:
> No point DCHECKing a pointer immediately before dereferencing it!
> 
> In DEBUG builds scoped_ptr<> will actually DCHECK for you, in fact.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:75: DCHECK(client_);
On 2015/03/16 22:19:09, Wez wrote:
> See above re DCHECK

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:93: // members are
destroyed via DeleteSoon on the message loop's TaskRunner.
On 2015/03/16 22:19:09, Wez wrote:
> Unless you actually run the message-loop after destroying the client, though,
it
> won't execute any tasks that are posted to it.
> 
> If you want that to happen then I'd suggest adding:
> 
> base::RunLoop().RunUntilIdle();
> 
> between the |client_| and |message_loop_| reset() calls.

I had thought this was happening in the D'Tor for |client_| but I was wrong,
added it here.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:106: done_closure_ =
run_loop.QuitClosure();
On 2015/03/16 22:19:08, Wez wrote:
> Why are you saving the QuitClosure() for a RunLoop you've allocated on the
stack
> into an object member variable?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:112: max_wait_time);
On 2015/03/16 22:19:09, Wez wrote:
> Cleaner to use a base::Timer for this?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:139: done_closure_ =
run_loop.QuitClosure();
On 2015/03/16 22:19:09, Wez wrote:
> See above, re why this needs saving.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:151: wait_time);
On 2015/03/16 22:19:09, Wez wrote:
> See above re base::Timer

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:161: void
AppRemotingConnectedClientTests::ConnectionStateChanged(
On 2015/03/16 22:19:08, Wez wrote:
> The RemoteConnectionObserver APIs are called by the |client_| - what thread
will
> that use? Consider DCHECKing that the thread is the same as the one on which
the
> test-case was instantiated.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:172: error_code !=
protocol::OK)) {
On 2015/03/16 22:19:08, Wez wrote:
> Why are these two connection-state checks different? e.g. When would we ever
> want to transition into the ready-for-tests=false state without quitting the
> test run-loop?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:177: void
AppRemotingConnectedClientTests::ConnectionReady(bool ready) {
On 2015/03/16 22:19:09, Wez wrote:
> Under what circumstances will we receive this call with |ready| false? Should
we
> be running |done_callback_| in those cases?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:185: if
(!host_message_received_callback_.is_null()) {
On 2015/03/16 22:19:09, Wez wrote:
> Do we really want to ignore host messages received while we're not expecting
> them?

I've updated this method so that we can add default message handlers to the
fixture (along with some logging to indicate when messages are received which
aren't handled) such as the OnWindowAddedHandler.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:199: // next send the host
a description of the client screen size.
On 2015/03/16 22:19:09, Wez wrote:
> nit: Capitalization

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:214: 
On 2015/03/16 22:19:08, Wez wrote:
> nit: No need for this blank line.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:216:
video_control.set_lossless_color(true);
On 2015/03/16 22:19:08, Wez wrote:
> By default these are currently off (false).

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:223: const std::string&
main_window_name,
On 2015/03/16 22:19:08, Wez wrote:
> It seems strange to pass main_window_name in as a bound parameter, even though
> this handler function is a member of the class, so has access to members of
the
> test instance object itself.

This is an artifact of some refactoring I did, you are totally right that it
doesn't make sense.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:230: LOG(ERROR) << "Message
received was not valid JSON.";
On 2015/03/16 22:19:08, Wez wrote:
> Should we terminate the test (e.g. via |done_callback_| in this case?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:239: << "underlying
application is in a bad state, YMMV.";
On 2015/03/16 22:19:08, Wez wrote:
> I'm surprised that this is ever a state we get into! Is this something you see
> in real test runs?

I saw this last year when we had created a snapshot where the host kept
crashing.  In that scenario the app would start up but the Host Process window
would pop and show up in front of the application window.  I'd like to log this
as it is a bad thing, I'm thinking it should be logged as an error though.  I'll
update the severity.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:250: FROM_HERE,
done_closure_, wait_time);
On 2015/03/16 22:19:08, Wez wrote:
> Consider replacing this with a base::Timer member of the test object.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.cc:252: }
On 2015/03/16 22:19:08, Wez wrote:
> Do we want to completely ignore all other window messages? Or at least verify
> that they are one of the ones we expect / know about, and fail otherwise?

I'd like to keep this method simple for now, we can add more logic later if we
want to handle sequences of messages.  Once this tool is in place, I am going to
gather scenarios and figure out what else we would need.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_connected_client_tests.h (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:30: // Creates a connection
to a remote host when the static SetUpTestCase() method
On 2015/03/16 22:19:10, Wez wrote:
> This comment should describe the overall intended purpose of this fixture
(i.e.
> to set up common test state such as the connection to the host) and then
> describe the things it does to achieve that, if you need to; as it is it is
very
> implementation-centric!

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:32: // This means that tests
which use this fixture can rely on having a connection
On 2015/03/16 22:19:10, Wez wrote:
> "This means that" is superfluous.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:35: // name of the
application we want to test.
On 2015/03/16 22:19:10, Wez wrote:
> This sentence is pretty confusingly worded!

My mastery of the English language changes depending on how much sleep my little
one allows me to have :)  I'll rewrite this and see if it is more coherent.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:36: class
AppRemotingConnectedClientTests
On 2015/03/16 22:19:09, Wez wrote:
> Why is this class "*Tests" rather than "*Test"? Each derived instance is a
> single test, isn't it, not multiple tests?

I've seen 'Tests' used for fixtures but it looks like Fixture is more popular in
chromium, I'll update the name.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:43: // static testing::Test
overrides.
On 2015/03/16 22:19:09, Wez wrote:
> Since these are static, they are presumably overrides only for this
> instantiation of the  testing::TestWithParam template?

Acknowledged.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:71: // Starts a connection
with the remote host defined by the parameter..
On 2015/03/16 22:19:09, Wez wrote:
> typo: spurious .

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:79: // for the title is
passed in.
On 2015/03/16 22:19:09, Wez wrote:
> This comment is confusing since it doesn't relate anything back to |message|
> directly, and its name doesn't seem to fit its function, going by the comment
-
> it seems that this method actually calls |done_closure_| if the contents of
> |message| match |main_window_name|, in which case shouldn't this be
> CallDoneCallbackIfWindowMatches() or similar?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:92:
HostMessageReceivedCallback host_message_received_callback_;
On 2015/03/16 22:19:09, Wez wrote:
> It's not clear how tests can customize this, since it's a private member and
> there are no protected nor public APIs to set it?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_connected_client_tests.h:103: static
scoped_ptr<TestChromotingClient> client_;
On 2015/03/16 22:19:09, Wez wrote:
> Do we really want to re-use a connection across tests?
> 
> That means tests may acquire implicit dependencies upon one another and break
if
> they are re-ordered, for example.
> 
> Is the test framework single-threaded? Or might it run multiple fixtures on
> different threads in the same process, at the same time?
> 
> Also note that we don't typically use scoped_ptr<> in statics in Chromium
code,
> since non-POD global variables cause the compiler to spit out initializer and
> finalizer code, and that makes process startup & shutdown slower, amongst
other
> things. It can also lead to issues if any global statics end up depending on,
or
> are depended on, by other classes.

I think I started out with a more complicated 'first test case' than is
necessary.  I will update the fixture to remove the statics and we can revisit
this if the usage of the tool by the rest of the team shows there is a problem
here worth solving.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.cc:22: const char
kHostAvailabilityFormatString[] = "%-25s%-40s%-10s\n";
On 2015/03/16 22:19:10, Wez wrote:
> You can define this immediately before the printf where you use it (and then
you
> don't need an anonymous namespace (which you didn't strictly need anyway,
since
> it's const :))

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.cc:164:
info.application_id.c_str(), status.c_str());
On 2015/03/16 22:19:10, Wez wrote:
> Is it normal to use printf() in test fixtures, rather than LOG() etc?

Done.  I used printf in the main cc file and had figured this would be displayed
in the same way, I'm fine logging to the LOG function though since everything
else is displayed that way.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.h (left):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.h:68: // |remote_host_info|
is not modified on failure.
On 2015/03/16 22:19:10, Wez wrote:
> RemoteHostInfo _is_ now modified on failure, then?

It is, I was treating 'pending' as a failure however I think it is important to
return those details to the caller who is going to call 'IsReadyForConnection'
anyway which will still return false.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.h (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.h:43: // their availability.
On 2015/03/16 22:19:10, Wez wrote:
> Displays it where?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
File remoting/test/remote_application_data.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.cc:17: return
remoting::test::kRemoteApplicationNames;
On 2015/03/16 22:19:10, Wez wrote:
> Why do you want to return an empty vector here..?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.cc:23: return
remoting::test::kRemoteApplicationInfoMap;
On 2015/03/16 22:19:10, Wez wrote:
> Why do you want to return an empty map here...?

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
File remoting/test/remote_application_data.h (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.h:6: #define
REMOTING_TEST_REMOTE_APPLICATION_DATA_H_
On 2015/03/16 22:19:10, Wez wrote:
> nit: Make the filename & guard macro consistent with the struct name, below.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.h:28: const std::vector<std::string>&
GetRemoteApplicationNames();
On 2015/03/16 22:19:10, Wez wrote:
> It's confusing that you refer to this as the "application name", "window name"
> or "main window name" in different places.
> 
> As a result it's not clear whether this function returns actual application
> names, application Ids, the names of all application windows, or the names
only
> of each application's "main" window.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_applic...
remoting/test/remote_application_data.h:31: const std::map<std::string,
ApplicationInfo>& GetRemoteApplicationInfoMap();
On 2015/03/16 22:19:10, Wez wrote:
> Again, it's not clear what the "name" is in this index, versus Id, window
name,
> etc.

Done.

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_host_i...
File remoting/test/remote_host_info_fetcher.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/remote_host_i...
remoting/test/remote_host_info_fetcher.cc:73:
request_->SetRequestContext(request_context_getter_.get());
On 2015/03/16 22:19:10, Wez wrote:
> SetRequestContext takes a reference to the getter (the API should really be
> updated to take a const scoped_refptr<>&...) , so you can just pass it in
> without keeping a reference to it yourself.

Discussed offline, a reference needs to be held until the request completes :(

[joedow, 2015-03-19 18:27:10]

Ping :)

[joedow, 2015-03-20 18:19:32]

ping ping! :)

[Wez, 2015-03-21 00:14:01]

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.cc:164:
info.application_id.c_str(), status.c_str());
On 2015/03/18 20:13:09, joedow wrote:
> On 2015/03/16 22:19:10, Wez wrote:
> > Is it normal to use printf() in test fixtures, rather than LOG() etc?
> 
> Done.  I used printf in the main cc file and had figured this would be
displayed
> in the same way, I'm fine logging to the LOG function though since everything
> else is displayed that way.

SGTM, although when I wrote that I hadn't realised that this is basically a
usage printout helper function - so printf() may make more sense here - up to
you.

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
File remoting/test/access_token_fetcher.cc (right):

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
remoting/test/access_token_fetcher.cc:25: // Factory function used to initialize
an app remoting scope vector.
nit: This is confusing since it almost reads like "scoped vector", which is
something else in Chromium!

Suggest rewording to "Returns a vector of the App Remoting auth scopes."

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
remoting/test/access_token_fetcher.cc:51: oauth_scopes_vector_ =
MakeAppRemotingScopeVector();
Why do you need to save this into a member variable, given that all you do is
Bind() that into the RefreshToken() call, below?

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
File remoting/test/access_token_fetcher.h (right):

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
remoting/test/access_token_fetcher.h:77: std::vector<std::string>
oauth_scopes_vector_;
These seem to be used only in the refresh token -> access token mode of use?

I thought that scopes were associated with refresh tokens, so that down-scoping
in this way actually creates new refresh tokens internally in GAIA?

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/app_remot...
File remoting/test/app_remoting_test_driver_environment.cc (right):

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/app_remot...
remoting/test/app_remoting_test_driver_environment.cc:147:
application_details_map_.at(application_name);
This DCHECK + fetch mirrors what GetDetailsFromAppName() already does, so use
that directly?

[joedow, 2015-03-21 01:23:59]

Addressed the 2nd round of feedback, PTAL.

Thanks!
Joe

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
File remoting/test/app_remoting_test_driver_environment.cc (right):

https://codereview.chromium.org/1008043003/diff/1/remoting/test/app_remoting_...
remoting/test/app_remoting_test_driver_environment.cc:164:
info.application_id.c_str(), status.c_str());
On 2015/03/21 00:14:01, Wez wrote:
> On 2015/03/18 20:13:09, joedow wrote:
> > On 2015/03/16 22:19:10, Wez wrote:
> > > Is it normal to use printf() in test fixtures, rather than LOG() etc?
> > 
> > Done.  I used printf in the main cc file and had figured this would be
> displayed
> > in the same way, I'm fine logging to the LOG function though since
everything
> > else is displayed that way.
> 
> SGTM, although when I wrote that I hadn't realised that this is basically a
> usage printout helper function - so printf() may make more sense here - up to
> you.

Acknowledged.

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
File remoting/test/access_token_fetcher.cc (right):

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
remoting/test/access_token_fetcher.cc:25: // Factory function used to initialize
an app remoting scope vector.
On 2015/03/21 00:14:01, Wez wrote:
> nit: This is confusing since it almost reads like "scoped vector", which is
> something else in Chromium!
> 
> Suggest rewording to "Returns a vector of the App Remoting auth scopes."

Removed since it wasn't needed : )

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
remoting/test/access_token_fetcher.cc:51: oauth_scopes_vector_ =
MakeAppRemotingScopeVector();
On 2015/03/21 00:14:01, Wez wrote:
> Why do you need to save this into a member variable, given that all you do is
> Bind() that into the RefreshToken() call, below?

Acknowledged.

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
File remoting/test/access_token_fetcher.h (right):

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/access_to...
remoting/test/access_token_fetcher.h:77: std::vector<std::string>
oauth_scopes_vector_;
On 2015/03/21 00:14:01, Wez wrote:
> These seem to be used only in the refresh token -> access token mode of use?
> 
> I thought that scopes were associated with refresh tokens, so that
down-scoping
> in this way actually creates new refresh tokens internally in GAIA?

Wow, great catch.  You are correct, This is not needed at all since we have the
scopes set in the original auth code URL.

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/app_remot...
File remoting/test/app_remoting_test_driver_environment.cc (right):

https://codereview.chromium.org/1008043003/diff/20001/remoting/test/app_remot...
remoting/test/app_remoting_test_driver_environment.cc:147:
application_details_map_.at(application_name);
On 2015/03/21 00:14:01, Wez wrote:
> This DCHECK + fetch mirrors what GetDetailsFromAppName() already does, so use
> that directly?

Done.

[joedow, 2015-03-23 23:04:33]

Ping!

[joedow, 2015-03-24 22:33:40]

Ping Ping!

[Wez, 2015-03-25 00:09:29]

lgtm

[joedow, 2015-03-25 00:33:05]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2015-03-25 00:34:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008043003/40001

[commit-bot: I haz the power, 2015-03-25 06:10:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-25 06:11:18]

Patchset 3 (id:??) landed as
https://crrev.com/2230e1f901c84ce7fb9a69b2055ba9879a0461a2
Cr-Commit-Position: refs/heads/master@{#322119}