Cleanup inconsistencies between GN and GYP in //remoting

remoting/remoting_test.gypi

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 {
   'targets': [
     {
       # GN version: //remoting:test_support
-      'target_name': 'remoting_test_common',
+      'target_name': 'remoting_test_support',
       'type': 'static_library',
       'dependencies': [
         '../base/base.gyp:base',
         '../components/components.gyp:policy_component_test_support',
         '../net/net.gyp:net_test_support',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
         'remoting_base',
         'remoting_client',
         'remoting_host',
         'remoting_protocol',
         'remoting_resources',
       ],
       'sources': [
         # Note: sources list duplicated in GN build.
         'host/fake_desktop_capturer.cc',
         'host/fake_desktop_capturer.h',
         'host/fake_desktop_environment.cc',
         'host/fake_desktop_environment.h',
+        'host/fake_host_extension.cc',
+        'host/fake_host_extension.h',
         'host/fake_host_status_monitor.h',
         'host/fake_mouse_cursor_monitor.cc',
         'host/fake_mouse_cursor_monitor.h',
+        'host/fake_oauth_token_getter.cc',
+        'host/fake_oauth_token_getter.h',
         'protocol/fake_authenticator.cc',
         'protocol/fake_authenticator.h',
         'protocol/fake_connection_to_host.cc',
         'protocol/fake_connection_to_host.h',
         'protocol/fake_datagram_socket.cc',
         'protocol/fake_datagram_socket.h',
         'protocol/fake_session.cc',
         'protocol/fake_session.h',
         'protocol/fake_stream_socket.cc',
         'protocol/fake_stream_socket.h',
         'protocol/protocol_mock_objects.cc',
         'protocol/protocol_mock_objects.h',
         'protocol/test_event_matchers.h',
         'signaling/fake_signal_strategy.cc',
         'signaling/fake_signal_strategy.h',
         'signaling/mock_signal_strategy.cc',
         'signaling/mock_signal_strategy.h',
         'test/access_token_fetcher.cc',
         'test/access_token_fetcher.h',
+        'test/app_remoting_connected_client_fixture.cc',
+        'test/app_remoting_connected_client_fixture.h',
+        'test/app_remoting_connection_helper.cc',
+        'test/app_remoting_connection_helper.h',
+        'test/app_remoting_latency_test_fixture.cc',
+        'test/app_remoting_latency_test_fixture.h',
         'test/app_remoting_report_issue_request.cc',
         'test/app_remoting_report_issue_request.h',
         'test/app_remoting_service_urls.cc',
         'test/app_remoting_service_urls.h',
         'test/app_remoting_test_driver_environment.cc',
+        'test/app_remoting_test_driver_environment.h',
         'test/app_remoting_test_driver_environment_app_details.cc',
-        'test/app_remoting_test_driver_environment.h',
-        'test/host_info.cc',
-        'test/host_info.h',
-        'test/host_list_fetcher.cc',
-        'test/host_list_fetcher.h',
         'test/fake_access_token_fetcher.cc',
         'test/fake_access_token_fetcher.h',
         'test/fake_app_remoting_report_issue_request.cc',
         'test/fake_app_remoting_report_issue_request.h',
         'test/fake_network_dispatcher.cc',
         'test/fake_network_dispatcher.h',
         'test/fake_network_manager.cc',
         'test/fake_network_manager.h',
         'test/fake_port_allocator.cc',
         'test/fake_port_allocator.h',
         'test/fake_remote_host_info_fetcher.cc',
         'test/fake_remote_host_info_fetcher.h',
         'test/fake_socket_factory.cc',
         'test/fake_socket_factory.h',
+        'test/host_info.cc',
+        'test/host_info.h',
+        'test/host_list_fetcher.cc',
+        'test/host_list_fetcher.h',
         'test/leaky_bucket.cc',
         'test/leaky_bucket.h',
         'test/mock_access_token_fetcher.cc',
         'test/mock_access_token_fetcher.h',
         'test/refresh_token_store.cc',
         'test/refresh_token_store.h',
         'test/remote_application_details.h',
         'test/remote_connection_observer.h',
         'test/remote_host_info.cc',
         'test/remote_host_info.h',
         'test/remote_host_info_fetcher.cc',
         'test/remote_host_info_fetcher.h',
         'test/test_chromoting_client.cc',
         'test/test_chromoting_client.h',
         'test/test_video_renderer.cc',
         'test/test_video_renderer.h',
       ],
       'conditions': [
         ['enable_remoting_host == 0', {
           'dependencies!': [
             'remoting_host',
           ],
           'sources/': [
             ['exclude', '^host/'],
           ]
         }],
       ],
     },
     {
-      'target_name': 'remoting_test_driver_common',
-      'type': 'static_library',
-      'dependencies': [
-        '../base/base.gyp:base',
-        '../base/base.gyp:test_support_base',
-        '../google_apis/google_apis.gyp:google_apis',
-        '../net/net.gyp:net',
-        '../remoting/remoting.gyp:remoting_base',
-        '../remoting/remoting.gyp:remoting_client',
-        '../remoting/remoting.gyp:remoting_protocol',
-        '../testing/gtest.gyp:gtest',
-      ],
-      'defines': [
-        'VERSION=<(version_full)',
-      ],
-      'sources': [
-        'test/access_token_fetcher.cc',
-        'test/access_token_fetcher.h',
-        'test/refresh_token_store.cc',
-        'test/refresh_token_store.h',
-        'test/remote_connection_observer.h',
-        'test/test_chromoting_client.cc',
-        'test/test_chromoting_client.h',
-        'test/test_video_renderer.cc',
-        'test/test_video_renderer.h',
-      ],
-    }, # end of target 'remoting_test_driver_common'
-    {
-      # A chromoting version of remoting_test_driver_common
       'target_name': 'chromoting_test_driver',
       'type': '<(gtest_target_type)',
       'dependencies': [
-        'remoting_test_driver_common',
-      ],
-      'defines': [
-        'VERSION=<(version_full)',
+        'remoting_test_support',
+        '../testing/gtest.gyp:gtest',
       ],
       'sources': [
-        'test/host_info.cc',
-        'test/host_info.h',
-        'test/host_list_fetcher.cc',
-        'test/host_list_fetcher.h',
         'test/chromoting_test_driver.cc',
       ],
-      'include_dirs': [
-        '../testing/gtest/include',
-      ],
     }, # end of target 'chromoting_test_driver'
     {
-      'target_name': 'ar_test_driver_common',

[joedow, 2015/07/16 02:45:14]

Removing this target will affect the internal ar_test_driver target, and
including remoting_test_support won't work in this change because the internal
target pulls in an internal *app_details.cc file so the public app_details.cc
file may need to get moved down to ar_sample_test_driver target

[Sergey Ulanov, 2015/07/16 17:39:11]

I see. I've added ar_test_driver_common back, but now it depends on
remoting_test_support. This still avoids compiling some files twice for
different targets.

But I don't think this kind of dependencies between targets is a good practice.
Essentially here we have target B that depends on A, but some code in A also
depends on code in B, i.e. we've got circular dependencies between the two
target. Not only it makes it harder to make changes in the build configuration
(because there is an implicit dependency between files), but also makes the code
harder to read. Can we avoid this? E.g. maybe pass the initialization hooks to
the AppRemotingTestDriverEnvironment constructor. Or you can make
PopulateApplicationNames() and PopulateApplicationDetailsMap() pure virtual and
override them in app-specific child classes.

-      'type': 'static_library',
-      'dependencies': [
-        '../remoting/proto/chromotocol.gyp:chromotocol_proto_lib',
-        '<(DEPTH)/third_party/webrtc/modules/modules.gyp:desktop_capture',
-        'remoting_test_driver_common',
-      ],
-      'defines': [
-        'VERSION=<(version_full)',
-      ],
-      'sources': [
-        'test/app_remoting_connected_client_fixture.cc',
-        'test/app_remoting_connected_client_fixture.h',
-        'test/app_remoting_connection_helper.cc',
-        'test/app_remoting_connection_helper.h',
-        'test/app_remoting_latency_test_fixture.cc',
-        'test/app_remoting_latency_test_fixture.h',
-        'test/app_remoting_report_issue_request.cc',
-        'test/app_remoting_report_issue_request.h',
-        'test/app_remoting_service_urls.cc',
-        'test/app_remoting_service_urls.h',
-        'test/app_remoting_test_driver_environment.cc',
-        'test/app_remoting_test_driver_environment.h',
-        'test/remote_application_details.h',
-        'test/remote_host_info.cc',
-        'test/remote_host_info.h',
-        'test/remote_host_info_fetcher.cc',
-        'test/remote_host_info_fetcher.h',
-      ],
-      'include_dirs': [
-        '../testing/gtest/include',
-      ],
-    },  # end of target 'ar_test_driver_common'
-    {
       # An external version of the test driver tool which includes minimal tests
       'target_name': 'ar_sample_test_driver',
       'type': '<(gtest_target_type)',
       'dependencies': [
-        'ar_test_driver_common',
-      ],
-      'defines': [
-        'VERSION=<(version_full)',
+        'remoting_test_support',
+        '../testing/gtest.gyp:gtest',
       ],
       'sources': [
         'test/app_remoting_test_driver.cc',
-        'test/app_remoting_test_driver_environment_app_details.cc',
-      ],
-      'include_dirs': [
-        '../testing/gtest/include',
       ],
     },  # end of target 'ar_sample_test_driver'
 
     # Remoting unit tests
     {
       # GN version: //remoting:remoting_unittests
       # Note that many of the sources are broken out into subdir-specific unit
       # test source set targets that then GN version then brings together.
       'target_name': 'remoting_unittests',
       'type': '<(gtest_target_type)',
@@ skipping 15 matching lines @@
         'remoting_base',
         'remoting_breakpad',
         'remoting_client',
         'remoting_client_plugin',
         'remoting_host',
         'remoting_host_setup_base',
         'remoting_it2me_host_static',
         'remoting_native_messaging_base',
         'remoting_protocol',
         'remoting_resources',
-        'remoting_test_common',
+        'remoting_test_support',
       ],
       'defines': [
         'VERSION=<(version_full)',
       ],
       'include_dirs': [
         '../testing/gmock/include',
       ],
       'sources': [
         # Note: sources list duplicated in GN build.
         'base/auto_thread_task_runner_unittest.cc',
@@ skipping 33 matching lines @@
         'host/capture_scheduler_unittest.cc',
         'host/chromeos/aura_desktop_capturer_unittest.cc',
         'host/chromeos/clipboard_aura_unittest.cc',
         'host/chromoting_host_context_unittest.cc',
         'host/chromoting_host_unittest.cc',
         'host/client_session_unittest.cc',
         'host/config_file_watcher_unittest.cc',
         'host/daemon_process_unittest.cc',
         'host/desktop_process_unittest.cc',
         'host/desktop_shape_tracker_unittest.cc',
-        'host/fake_desktop_capturer.cc',
-        'host/fake_desktop_capturer.h',
-        'host/fake_host_extension.cc',
-        'host/fake_host_extension.h',
-        'host/fake_host_status_monitor.h',
-        'host/fake_oauth_token_getter.cc',
-        'host/fake_oauth_token_getter.h',
         'host/gcd_rest_client_unittest.cc',
         'host/gcd_state_updater_unittest.cc',
         'host/gnubby_auth_handler_posix_unittest.cc',
         'host/heartbeat_sender_unittest.cc',
         'host/host_change_notification_listener_unittest.cc',
         'host/host_config_unittest.cc',
         'host/host_extension_session_manager_unittest.cc',
         'host/host_mock_objects.cc',
         'host/host_status_logger_unittest.cc',
         'host/ipc_desktop_environment_unittest.cc',
@@ skipping 263 matching lines @@
         {
           'target_name': 'remoting_perftests',
           'type': '<(gtest_target_type)',
           'dependencies': [
             '../base/base.gyp:base',
             '../base/base.gyp:test_support_base',
             '../testing/gtest.gyp:gtest',
             '../third_party/webrtc/modules/modules.gyp:desktop_capture',
             '../third_party/libjingle/libjingle.gyp:libjingle',
             'remoting_base',
-            'remoting_test_common',
+            'remoting_test_support',
           ],
           'defines': [
             'VERSION=<(version_full)',
           ],
           'include_dirs': [
             '../testing/gmock/include',
           ],
           'sources': [
             'base/run_all_unittests.cc',
             'codec/codec_test.cc',
@@ skipping 59 matching lines @@
             '../build/isolate.gypi',
           ],
           'sources': [
             'remoting_unittests.isolate',
           ],
         },
       ],
     }],
   ] # end of 'conditions'
 }