Cleanup inconsistencies between GN and GYP in //remoting

remoting/remoting_test.gypi

Index: remoting/remoting_test.gypi
diff --git a/remoting/remoting_test.gypi b/remoting/remoting_test.gypi
index 73a9f17448e9a03babdcf19310f88eb8e52c8032..20945c98ef77876696775ae65b19c64ff4d395fa 100644
--- a/remoting/remoting_test.gypi
+++ b/remoting/remoting_test.gypi
@@ -6,7 +6,7 @@
   'targets': [
     {
       # GN version: //remoting:test_support
-      'target_name': 'remoting_test_common',
+      'target_name': 'remoting_test_support',
       'type': 'static_library',
       'dependencies': [
         '../base/base.gyp:base',
@@ -26,9 +26,13 @@
         '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',
@@ -48,17 +52,19 @@
         '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_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/app_remoting_test_driver_environment_app_details.cc',
         'test/fake_access_token_fetcher.cc',
         'test/fake_access_token_fetcher.h',
         'test/fake_app_remoting_report_issue_request.cc',
@@ -73,6 +79,10 @@
         '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',
@@ -102,104 +112,26 @@
       ],
     },
     {
-      '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'
 
@@ -235,7 +167,7 @@
         'remoting_native_messaging_base',
         'remoting_protocol',
         'remoting_resources',
-        'remoting_test_common',
+        'remoting_test_support',
       ],
       'defines': [
         'VERSION=<(version_full)',
@@ -289,13 +221,6 @@
         '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',
@@ -579,7 +504,7 @@
             '../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)',