Gabriela Gibson wrote on Mon, Apr 01, 2013 at 22:36:57 +0100:
> [[[
>
> Disable ANSI color for dumb terminals, fix logic bug where
> dimensions of (0,0) was treated as a positive result,
> re-factored _run_tests to remove repeated calculation of
> line_length.
>
> * build/run_tests.py
> (_get_term_width): Add test condition.
> (TestHarness): Add new variable.
> (_run_test): Add new parameter, remove function call.
>
> ]]]
>
>
> Index: build/run_tests.py
> ===================================================================
> --- build/run_tests.py (revision 1463012)
> +++ build/run_tests.py (working copy)
> @@ -100,6 +100,11 @@ def _get_term_width():
> os.close(fd)
> except:
> pass
> + ## Some terminals (eg emacs *shell*, emacs *compilation*)
> + ## seem to return (0,0) from ioctl_GWINSZ, so let's deal
> + ## with that case...
> + if cr == (0, 0):
> + cr = None
I'm not sure how to interpret a return value of (0,0) that's not
accompanied by an error flag (C errno!=0, or a Python exception). Is
that normal behaviour, a bug we should be working around in our code, or
an indication of a bug in our logic in the preceding lines?
i.e., if (0,0) means "_get_term_width() was going 35mph in a school zone
at the time of the call to fcntl.ioctl()", we should fix that. But if
(0,0) is just a terminal emulator bug, or expected behaviour, then +1 to
commit.
> if not cr:
> try:
> cr = (os.environ['LINES'], os.environ['COLUMNS'])
> @@ -178,7 +183,8 @@ class TestHarness:
> self.log = None
> self.ssl_cert = ssl_cert
> self.http_proxy = http_proxy
> - if not sys.stdout.isatty() or sys.platform == 'win32':
> + if not sys.stdout.isatty() or sys.platform == 'win32' or \
> + os.getenv("TERM") == "dumb":
> TextColors.disable()
>
> def run(self, list):
> @@ -186,8 +192,9 @@ class TestHarness:
> there is a log file. Return zero iff all test programs passed.'''
> self._open_log('w')
> failed = 0
> + line_length = _get_term_width()
> for cnt, prog in enumerate(list):
> - failed = self._run_test(prog, cnt, len(list)) or failed
> + failed = self._run_test(prog, cnt, len(list), line_length) or failed
>
This change will actually change the output if you resize the terminal
window while running tests interactively (at least in terminal emulators
that support the ioctl_GWINSZ approach). I'm not objected to it, but
it's not clear to me what it gains either (shaves 100 syscalls from
a 'make check' run?).
> if self.log is None:
> return failed
> @@ -550,7 +557,7 @@ class TestHarness:
>
> return failed
>
> - def _run_test(self, prog, test_nr, total_tests):
> + def _run_test(self, prog, test_nr, total_tests, line_length):
> "Run a single test. Return the test's exit code."
>
> if self.log:
> @@ -587,7 +594,6 @@ class TestHarness:
>
> progabs = os.path.abspath(os.path.join(self.srcdir, prog))
> old_cwd = os.getcwd()
> - line_length = _get_term_width()
> dots_needed = line_length \
> - len(test_info) \
> - len('success')
