Bug 1175708 - Eliminate some horrid action-at-a-distance global state in jstests.py; r=sfink
authorTerrence Cole <terrence@mozilla.com>
Wed, 17 Jun 2015 14:29:53 -0700
changeset 281977 ede9f497a5cc70239a2b7aa9e9f071b46f1089eb
parent 281976 f08884c972231f6a0bf38dbd3758bbb86966b2b6
child 281978 aa043f7e701fb4e87a2009e233d4789ebd421c49
push id897
push userjlund@mozilla.com
push dateMon, 14 Sep 2015 18:56:12 +0000
treeherdermozilla-release@9411e2d2b214 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1175708
milestone41.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1175708 - Eliminate some horrid action-at-a-distance global state in jstests.py; r=sfink
js/src/tests/jstests.py
js/src/tests/lib/tasks_unix.py
js/src/tests/lib/tasks_win.py
js/src/tests/lib/tests.py
--- a/js/src/tests/jstests.py
+++ b/js/src/tests/jstests.py
@@ -4,38 +4,30 @@ The JS Shell Test Harness.
 
 See the adjacent README.txt for more details.
 """
 
 from __future__ import print_function
 
 import os, sys, textwrap
 from os.path import abspath, dirname, isfile, realpath
+from contextlib import contextmanager
 from copy import copy
 from subprocess import list2cmdline, call
 
 from lib.results import NullTestOutput
 from lib.tests import TestCase, get_jitflags
 from lib.results import ResultsSink
 from lib.progressbar import ProgressBar
 
 if sys.platform.startswith('linux') or sys.platform.startswith('darwin'):
     from lib.tasks_unix import run_all_tests
 else:
     from lib.tasks_win import run_all_tests
 
-def run_tests(options, tests, results):
-    """Run the given tests, sending raw results to the given results
-    accumulator."""
-    try:
-        completed = run_all_tests(tests, results, options)
-    except KeyboardInterrupt:
-        completed = False
-
-    results.finish(completed)
 
 def get_cpu_count():
     """
     Guess at a reasonable parallelism count to set as the default for the
     current machine and run.
     """
     # Python 2.6+
     try:
@@ -57,16 +49,27 @@ def get_cpu_count():
         res = int(os.environ['NUMBER_OF_PROCESSORS'])
         if res > 0:
             return res
     except (KeyError, ValueError):
         pass
 
     return 1
 
+
+@contextmanager
+def changedir(dirname):
+    pwd = os.getcwd()
+    os.chdir(dirname)
+    try:
+        yield
+    finally:
+        os.chdir(pwd)
+
+
 def parse_args():
     """
     Parse command line arguments.
     Returns a tuple of: (options, js_shell, requested_paths, excluded_paths)
         options :object: The raw OptionParser output.
         js_shell :str: The absolute location of the shell to test with.
         requested_paths :set<str>: Test paths specially requested on the CLI.
         excluded_paths :set<str>: Test paths specifically excluded by the CLI.
@@ -186,35 +189,36 @@ def parse_args():
     if options.js_shell is None and not options.make_manifests:
         op.error('missing JS_SHELL argument')
 
     # Valgrind, gdb, and rr are mutually exclusive.
     if sum(map(lambda e: 1 if e else 0, [options.valgrind, options.debug, options.rr])) > 1:
         op.error("--valgrind, --debug, and --rr are mutually exclusive.")
 
     # Fill the debugger field, as needed.
-    prefix = options.debugger.split() if options.debug else []
+    debugger_prefix = options.debugger.split() if options.debug else []
     if options.valgrind:
-        prefix = ['valgrind'] + options.valgrind_args.split()
+        debugger_prefix = ['valgrind'] + options.valgrind_args.split()
         if os.uname()[0] == 'Darwin':
-            prefix.append('--dsymutil=yes')
+            debugger_prefix.append('--dsymutil=yes')
         options.show_output = True
     if options.rr:
-        prefix = ['rr', 'record']
+        debugger_prefix = ['rr', 'record']
 
     js_cmd_args = options.shell_args.split()
     if options.jorendb:
         options.passthrough = True
         options.hide_progress = True
         options.worker_count = 1
         debugger_path = realpath(os.path.join(
             abspath(dirname(abspath(__file__))),
             '..', '..', 'examples', 'jorendb.js'))
         js_cmd_args.extend(['-d', '-f', debugger_path, '--'])
-    TestCase.set_js_cmd_prefix(options.js_shell, js_cmd_args, prefix)
+    prefix = TestCase.build_js_cmd_prefix(options.js_shell, js_cmd_args,
+                                          debugger_prefix)
 
     # If files with lists of tests to run were specified, add them to the
     # requested tests set.
     if options.test_file:
         for test_file in options.test_file:
             requested_paths |= set(
                 [line.strip() for line in open(test_file).readlines()])
 
@@ -243,17 +247,18 @@ def parse_args():
         except IOError as ex:
             raise SystemExit("Failed to open output file: " + str(ex))
 
     # Hide the progress bar if it will get in the way of other output.
     options.hide_progress = (options.format == 'automation' or
                              not ProgressBar.conservative_isatty() or
                              options.hide_progress)
 
-    return (options, requested_paths, excluded_paths)
+    return (options, prefix, requested_paths, excluded_paths)
+
 
 def load_tests(options, requested_paths, excluded_paths):
     """
     Returns a tuple: (skipped_tests, test_list)
         skip_list: [iterable<Test>] Tests found but skipped.
         test_list: [iterable<Test>] Tests found that should be run.
     """
     import lib.manifest as manifest
@@ -319,18 +324,19 @@ def load_tests(options, requested_paths,
         test_list = [_ for _ in test_list if not _.slow]
 
     if not options.run_skipped:
         skip_list = [_ for _ in test_list if not _.enable]
         test_list = [_ for _ in test_list if _.enable]
 
     return skip_list, test_list
 
+
 def main():
-    options, requested_paths, excluded_paths = parse_args()
+    options, prefix, requested_paths, excluded_paths = parse_args()
     if options.js_shell is not None and not isfile(options.js_shell):
         print('Could not find shell at given path.')
         return 1
     skip_list, test_list = load_tests(options, requested_paths, excluded_paths)
 
     if not test_list:
         print('no tests selected')
         return 1
@@ -343,38 +349,33 @@ def main():
                   ' debugger can only run one')
             for tc in test_list:
                 print('    {}'.format(tc.path))
             return 2
 
         cmd = test_list[0].get_command(TestCase.js_cmd_prefix)
         if options.show_cmd:
             print(list2cmdline(cmd))
-        if test_dir not in ('', '.'):
-            os.chdir(test_dir)
-        call(cmd)
+        with changedir(test_dir):
+            call(cmd)
         return 0
 
-    curdir = os.getcwd()
-    if test_dir not in ('', '.'):
-        os.chdir(test_dir)
-
-    # Force Pacific time zone to avoid failures in Date tests.
-    os.environ['TZ'] = 'PST8PDT'
-    # Force date strings to English.
-    os.environ['LC_TIME'] = 'en_US.UTF-8'
+    with changedir(test_dir):
+        # Force Pacific time zone to avoid failures in Date tests.
+        os.environ['TZ'] = 'PST8PDT'
+        # Force date strings to English.
+        os.environ['LC_TIME'] = 'en_US.UTF-8'
 
-    results = None
-    try:
         results = ResultsSink(options, len(skip_list) + len(test_list))
-        for t in skip_list:
-            results.push(NullTestOutput(t))
-        run_tests(options, test_list, results)
-    finally:
-        os.chdir(curdir)
+        try:
+            for t in skip_list:
+                results.push(NullTestOutput(t))
+            ok = run_all_tests(test_list, prefix, results, options)
+        except KeyboardInterrupt:
+            ok = False
 
-    if results is None or not results.all_passed():
-        return 1
+        results.finish(ok)
+        return 0 if results.all_passed() else 1
 
     return 0
 
 if __name__ == '__main__':
     sys.exit(main())
--- a/js/src/tests/lib/tasks_unix.py
+++ b/js/src/tests/lib/tasks_unix.py
@@ -2,48 +2,48 @@
 # waitpid to dispatch tasks.  This avoids several deadlocks that are possible
 # with fork/exec + threads + Python.
 
 import errno, os, select
 from datetime import datetime, timedelta
 from results import TestOutput
 
 class Task(object):
-    def __init__(self, test, pid, stdout, stderr):
+    def __init__(self, test, prefix, pid, stdout, stderr):
         self.test = test
-        self.cmd = test.get_command(test.js_cmd_prefix)
+        self.cmd = test.get_command(prefix)
         self.pid = pid
         self.stdout = stdout
         self.stderr = stderr
         self.start = datetime.now()
         self.out = []
         self.err = []
 
-def spawn_test(test, passthrough=False):
+def spawn_test(test, prefix, passthrough=False):
     """Spawn one child, return a task struct."""
     if not passthrough:
         (rout, wout) = os.pipe()
         (rerr, werr) = os.pipe()
 
         rv = os.fork()
 
         # Parent.
         if rv:
             os.close(wout)
             os.close(werr)
-            return Task(test, rv, rout, rerr)
+            return Task(test, prefix, rv, rout, rerr)
 
         # Child.
         os.close(rout)
         os.close(rerr)
 
         os.dup2(wout, 1)
         os.dup2(werr, 2)
 
-    cmd = test.get_command(test.js_cmd_prefix)
+    cmd = test.get_command(prefix)
     os.execvp(cmd[0], cmd)
 
 def total_seconds(td):
     """
     Return the total number of seconds contained in the duration as a float
     """
     return (float(td.microseconds) \
             + (td.seconds + td.days * 24 * 3600) * 10**6) / 10**6
@@ -175,27 +175,27 @@ def reap_zombies(tasks, results, timeout
 def kill_undead(tasks, results, timeout):
     """
     Signal all children that are over the given timeout.
     """
     for task in tasks:
         if timed_out(task, timeout):
             os.kill(task.pid, 9)
 
-def run_all_tests(tests, results, options):
+def run_all_tests(tests, prefix, results, options):
     # Copy and reverse for fast pop off end.
     tests = tests[:]
     tests.reverse()
 
     # The set of currently running tests.
     tasks = []
 
     while len(tests) or len(tasks):
         while len(tests) and len(tasks) < options.worker_count:
-            tasks.append(spawn_test(tests.pop(), options.passthrough))
+            tasks.append(spawn_test(tests.pop(), prefix, options.passthrough))
 
         timeout = get_max_wait(tasks, results, options.timeout)
         read_input(tasks, timeout)
 
         kill_undead(tasks, results, options.timeout)
         tasks = reap_zombies(tasks, results, options.timeout)
 
         results.pb.poke()
--- a/js/src/tests/lib/tasks_win.py
+++ b/js/src/tests/lib/tasks_win.py
@@ -13,28 +13,29 @@ class EndMarker:
 
 
 def _do_work(qTasks, qResults, timeout):
     while True:
         test = qTasks.get(block=True, timeout=sys.maxint)
         if test is EndMarker:
             qResults.put(EndMarker)
             return
-        qResults.put(test.run(test.js_cmd_prefix, timeout))
+        qResults.put(test.run(prefix, timeout))
 
 
-def run_all_tests_gen(tests, results, options):
+def run_all_tests_gen(tests, prefix, results, options):
     """
     Uses scatter-gather to a thread-pool to manage children.
     """
     qTasks, qResults = Queue(), Queue()
 
     workers = []
     for _ in range(options.worker_count):
-        worker = Thread(target=_do_work, args=(qTasks, qResults, options.timeout))
+        worker = Thread(target=_do_work, args=(qTasks, qResults, prefix,
+                                               options.timeout))
         worker.setDaemon(True)
         worker.start()
         workers.append(worker)
 
     # Insert all jobs into the queue, followed by the queue-end
     # marker, one per worker. This will not block on growing the
     # queue, only on waiting for more items in the generator. The
     # workers are already started, however, so this will process as
--- a/js/src/tests/lib/tests.py
+++ b/js/src/tests/lib/tests.py
@@ -113,31 +113,29 @@ class Test(object):
         """Return the '-f shell.js' options needed to run a test with the given
         path."""
         if path == '':
             return ['-f', 'shell.js']
         head, base = os.path.split(path)
         return Test.prefix_command(head) \
             + ['-f', os.path.join(path, 'shell.js')]
 
-    def get_command(self, js_cmd_prefix):
+    def get_command(self, prefix):
         dirname, filename = os.path.split(self.path)
-        cmd = js_cmd_prefix + self.jitflags + self.options \
+        cmd = prefix + self.jitflags + self.options \
               + Test.prefix_command(dirname) + ['-f', self.path]
         return cmd
 
-    def run(self, js_cmd_prefix, timeout=30.0):
-        cmd = self.get_command(js_cmd_prefix)
+    def run(self, prefix, timeout=30.0):
+        cmd = self.get_command(prefix)
         out, err, rc, dt, timed_out = run_cmd(cmd, timeout)
         return TestOutput(self, cmd, out, err, rc, dt, timed_out)
 
 class TestCase(Test):
     """A test case consisting of a test and an expected result."""
-    js_cmd_prefix = None
-
     def __init__(self, path):
         Test.__init__(self, path)
         self.enable = True   # bool: True => run test, False => don't run
         self.expect = True   # bool: expected result, True => pass
         self.random = False  # bool: True => ignore output as 'random'
         self.slow = False    # bool: True => test may run slowly
 
         # The terms parsed to produce the above properties.
@@ -158,25 +156,25 @@ class TestCase(Test):
         if self.random:
             ans += ', random'
         if self.slow:
             ans += ', slow'
         if '-d' in self.options:
             ans += ', debugMode'
         return ans
 
-    @classmethod
-    def set_js_cmd_prefix(self, js_path, js_args, debugger_prefix):
+    @staticmethod
+    def build_js_cmd_prefix(js_path, js_args, debugger_prefix):
         parts = []
         if debugger_prefix:
             parts += debugger_prefix
         parts.append(js_path)
         if js_args:
             parts += js_args
-        self.js_cmd_prefix = parts
+        return parts
 
     def __cmp__(self, other):
         if self.path == other.path:
             return 0
         elif self.path < other.path:
             return -1
         return 1