Force close of browser Windows when complete.

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
===================================================================
--- tools/testing/dart/test_runner.dart	(revision 4199)
+++ tools/testing/dart/test_runner.dart	(working copy)
@@ -510,6 +510,11 @@
   // configurations there is no need to repeatedly search the file
   // system, generate tests, and search test files for options.
   Map<String, List<TestInformation>> _testCache;
+  /**
+   * String indicating the browser used to run the tests. Empty if no browser 
+   * used.
+   */
+  String browserUsed;
 
   ProcessQueue(int this._maxProcesses,
                String progress,
@@ -526,6 +531,7 @@
         _batchProcesses = new List<DartcBatchRunnerProcess>(),
         _testCache = new Map<String, List<TestInformation>>() {
     if (!_enqueueMoreWork(this)) _progress.allDone();
+    browserUsed = '';
   }
 
   /**
@@ -535,6 +541,7 @@
     _activeTestListers++;
     testSuite.forEachTest(_runTest, _testCache, globalTemporaryDirectory,
                           _testListerDone);
+    testSuite.notifyIfBrowserCleanupNeeded(this);

[Jennifer Messerly, 2012/02/14 02:17:25]

an alternate idea: instead of changing test_suite, could you change _runTest in
this class?

From there you can access test.configuration, which should give you:

if (test is BrowserTestCase) { // or test.configuration['component'] ==
'webdriver'
  browserUsed = test.configuration['browser'];
}

[Emily Fortuna, 2012/02/14 23:55:44]

Done.

   }
 
   void _testListerDone() {
@@ -555,6 +562,44 @@
     return _temporaryDirectory;
   }
 
+  /**
+   * Sometimes Webdriver doesn't close every browser window when it's done 
+   * with a test. At the end of all tests we clear out any neglected processes
+   * that are still running. 
+   */
+  void killZombieBrowsers() {
+    String chromeName = 'chrome';
+    if (new Platform().operatingSystem() == 'macos') {
+      chromeName = 'Google\ Chrome';
+    }
+    Map<String, List<String>> processNames = {'ie': ['iexplore'], 'safari': 
+        ['Safari'], 'ff': ['firefox'], 'chrome': ['chromedriver', chromeName]};
+    if (new Platform().operatingSystem() == 'windows') {
+      for (String name in processNames[browserUsed]) {
+        Process process = new Process.start(
+            'C:\\Windows\\System32\\taskkill.exe', ['/F', '/IM', name + '.exe',
+            '/T']);
+        process.exitHandler = (exitCode) {
+          process.close();
+          _progress.allDone();

[Jennifer Messerly, 2012/02/14 02:17:25]

is it okay that we're potentially calling allDone here multiple times? in other
words, if chromedriver and chrome are being killed, each one will call
_progress.allDone()

[Bill Hesse, 2012/02/14 12:46:00]

I don't think this is OK.

+        };
+        process.errorHandler = (error) {
+          _progress.allDone();
+        };
+      }
+    } else {
+      for (String name in processNames[browserUsed]) {
+        Process process = new Process.start('killall', ['-9', name]);
+        process.exitHandler = (exitCode) {
+          process.close();
+          _progress.allDone();
+        };
+        process.errorHandler = (error) {
+          _progress.allDone();
+        };
+      }
+    }
+  }
 
   void _checkDone() {
     // When there are no more active test listers ask for more work
@@ -564,12 +609,20 @@
       if (_tests.isEmpty() && _numProcesses == 0) {
         _terminateDartcBatchRunners();
         if (_keepGeneratedTests || _temporaryDirectory == null) {
-          _progress.allDone();
+          if (browserUsed != '') {
+            killZombieBrowsers();
+          } else {
+            _progress.allDone();
+          }

[Jennifer Messerly, 2012/02/14 02:17:25]

it'd be good to pull this pattern out into a function

[Emily Fortuna, 2012/02/14 23:55:44]

Done.

         } else if (!_temporaryDirectory.startsWith('/tmp/') ||
                    _temporaryDirectory.contains('/../')) {
           // Let's be extra careful, since rm -rf is so dangerous.
           print('Temporary directory $_temporaryDirectory unsafe to delete!');
-          _progress.allDone();
+          if (browserUsed != '') {
+            killZombieBrowsers();
+          } else {
+            _progress.allDone();
+          }
         } else {
           // TODO(dart:1211): Use delete(recursive=true) in Dart when it is
           // implemented, and add Windows support.
@@ -583,7 +636,11 @@
             } else {
               print('\nDeletion of temp dir $_temporaryDirectory failed.');
             }
-            _progress.allDone();
+            if (browserUsed != '') {
+              killZombieBrowsers();
+            } else {
+              _progress.allDone();
+            }
           };
         }
       }