Don't log xvfb errors under normal circumstances in run-webkit-tests

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py

 # Copyright (C) 2010 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 98 matching lines @@
         _log.error('Could not find apache. Not installed or unknown path.')
         return None
 
     def setup_test_run(self):
         super(LinuxPort, self).setup_test_run()
         self._start_xvfb()
         self._setup_dummy_home_dir()
 
     def clean_up_test_run(self):
         super(LinuxPort, self).clean_up_test_run()
-        self._stop_xvfb()
+        self._stop_xvfb(save_logs=False)
         self._clean_up_dummy_home_dir()
 
     #
     # PROTECTED METHODS
     #
 
     def _setup_dummy_home_dir(self):
         """Creates a dummy home directory for running the test.
 
         This is a workaround for crbug.com/595504; see crbug.com/612730.
@@ skipping 14 matching lines @@
                 continue
             fs.copyfile(original_path, fs.join(dummy_home, filename))
 
     def _clean_up_dummy_home_dir(self):
         """Cleans up the dummy dir and resets the HOME environment variable."""
         dummy_home = self.host.environ['HOME']
         assert dummy_home != self._original_home
         self._filesystem.rmtree(dummy_home)
         self.host.environ['HOME'] = self._original_home
 
     def _start_xvfb(self):

[mithro, 2017/04/27 03:34:51]

I think one of the _log statements which describes which display was set and
xvfb was started should be an .info level message.

[Dirk Pranke, 2017/04/27 17:18:31]

I've tried pretty hard to keep run-webkit-tests as quiet as possible. I think
this is more of a debug-level message, since in normal operation the user
shouldn't need to know or care which port is being used.

[mithro, 2017/04/28 05:46:31]

I agree having the output for a human being readable and short is a good plan. 

However, when running on the bots it is really hard to know what is going wrong.
Maybe we could have a default output log which isn't stdout for this type of
output?

         display = self._find_display()
         if not display:
             _log.warn('Failed to find a free display to start Xvfb.')
             return
 
-        _log.info('Starting Xvfb with display "%s".', display)
+        _log.debug('Starting Xvfb with display "%s".', display)
         self._xvfb_stdout = tempfile.NamedTemporaryFile(delete=False)
         self._xvfb_stderr = tempfile.NamedTemporaryFile(delete=False)
         self._xvfb_process = self.host.executive.popen(
             ['Xvfb', display, '-screen', '0', '1280x800x24', '-ac', '-dpi', '96'],
             stdout=self._xvfb_stdout, stderr=self._xvfb_stderr)
 
         # By setting DISPLAY here, the individual worker processes will
         # get the right DISPLAY. Note, if this environment could be passed
         # when creating workers, then we wouldn't need to modify DISPLAY here.
         self._original_display = self.host.environ.get('DISPLAY')
         self.host.environ['DISPLAY'] = display
 
         # The poll() method will return None if the process has not terminated:
         # https://docs.python.org/2/library/subprocess.html#subprocess.Popen.poll
         if self._xvfb_process.poll() is not None:
             _log.warn('Failed to start Xvfb on display "%s."', display)
-            self._stop_xvfb()
+            self._stop_xvfb(save_logs=True)
 
         start_time = self.host.time()
         while self.host.time() - start_time < self.XVFB_START_TIMEOUT:

[mithro, 2017/04/27 03:34:51]

BTW - We should probably be checking self._xvfb_process.poll() == None in this
loop.

Shall I send out a CL for that?

[Dirk Pranke, 2017/04/27 17:18:31]

Sure.

             # We don't explicitly set the display, as we want to check the
             # environment value.
             exit_code = self.host.executive.run_command(
                 ['xdpyinfo'], return_exit_code=True)
             if exit_code == 0:
-                _log.info('Successfully started Xvfb with display "%s".', display)
+                _log.debug('Successfully started Xvfb with display "%s".', display)
                 return
             _log.warn('xdpyinfo check failed with exit code %s while starting Xvfb on "%s".', exit_code, display)
             self.host.sleep(0.1)
         _log.fatal('Failed to start Xvfb on display "%s" (xdpyinfo check failed).', display)
-        self._stop_xvfb()
+        self._stop_xvfb(save_logs=True)
 
     def _find_display(self):
         """Tries to find a free X display, looping if necessary."""
         # The "xvfb-run" command uses :99 by default.
         for display_number in range(99, 120):
             display = ':%d' % display_number
             exit_code = self.host.executive.run_command(
                 ['xdpyinfo', '-display', display], return_exit_code=True)
             if exit_code == 1:
                 return display
         return None
 
-    def _stop_xvfb(self):
+    def _stop_xvfb(self, save_logs):
         if self._original_display:
             self.host.environ['DISPLAY'] = self._original_display
         if self._xvfb_stdout:
             self._xvfb_stdout.close()
         if self._xvfb_stderr:
             self._xvfb_stderr.close()
         if self._xvfb_process:
             _log.debug('Killing Xvfb process pid %d.', self._xvfb_process.pid)
             self._xvfb_process.kill()
             self._xvfb_process.wait()
-        if self._xvfb_stdout and self.host.filesystem.exists(self._xvfb_stdout.name):
+        if save_logs and self._xvfb_stdout and self.host.filesystem.exists(self._xvfb_stdout.name):
             for line in self.host.filesystem.read_text_file(self._xvfb_stdout.name).splitlines():
                 _log.warn('Xvfb stdout:  %s', line)
             self.host.filesystem.remove(self._xvfb_stdout.name)
-        if self._xvfb_stderr and self.host.filesystem.exists(self._xvfb_stderr.name):
+        if save_logs and self._xvfb_stderr and self.host.filesystem.exists(self._xvfb_stderr.name):
             for line in self.host.filesystem.read_text_file(self._xvfb_stderr.name).splitlines():
                 _log.warn('Xvfb stderr:  %s', line)
             self.host.filesystem.remove(self._xvfb_stderr.name)
         self._xvfb_stdout = self._xvfb_stderr = self._xvfb_process = None
 
     def _path_to_driver(self, target=None):
         binary_name = self.driver_name()
         return self._build_path_with_target(target, binary_name)