Patchwork D1750: run-tests: extract sorting of tests to own function

login
register
mail settings
Submitter phabricator
Date Dec. 22, 2017, 8:02 p.m.
Message ID <differential-rev-PHID-DREV-fqi5cb6xr6ocu37mkwnc-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26412/
State Superseded
Headers show

Comments

phabricator - Dec. 22, 2017, 8:02 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  TestRunner._run() is a large function and is difficult to follow.
  Let's extract the test sorting to its own function to make it shorter.
  
  When I refactored run-tests.py several years ago, I put a lot of
  functionality in methods. The prevailing Mercurial style is to use
  functions - not classes - where possible. While refactoring the code,
  I decided to undo this historical mistake of mine by moving the code
  to a standalone function.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1750

AFFECTED FILES
  tests/run-tests.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 22, 2017, 8:45 p.m.
quark added inline comments.

INLINE COMMENTS

> run-tests.py:2271
>  
> +def sorttests(tests, shuffle=False):
> +    """Do an in-place sort of tests."""

I prefer the name `testdescs` since they are "test descriptions", not the real test objects. I think I renamed `tests` to `testdescs` intentionally.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1750

To: indygreg, #hg-reviewers
Cc: quark, mercurial-devel

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -2268,6 +2268,50 @@ 
                              separators=(',', ': '))
         outf.writelines(("testreport =", jsonout))
 
+def sorttests(tests, shuffle=False):
+    """Do an in-place sort of tests."""
+    if shuffle:
+        random.shuffle(tests)
+        return
+
+    # keywords for slow tests
+    slow = {b'svn': 10,
+            b'cvs': 10,
+            b'hghave': 10,
+            b'largefiles-update': 10,
+            b'run-tests': 10,
+            b'corruption': 10,
+            b'race': 10,
+            b'i18n': 10,
+            b'check': 100,
+            b'gendoc': 100,
+            b'contrib-perf': 200,
+            }
+    perf = {}
+
+    def sortkey(f):
+        # run largest tests first, as they tend to take the longest
+        f = f['path']
+        try:
+            return perf[f]
+        except KeyError:
+            try:
+                val = -os.stat(f).st_size
+            except OSError as e:
+                if e.errno != errno.ENOENT:
+                    raise
+                perf[f] = -1e9  # file does not exist, tell early
+                return -1e9
+            for kw, mul in slow.items():
+                if kw in f:
+                    val *= mul
+            if f.endswith(b'.py'):
+                val /= 10.0
+            perf[f] = val / 1000.0
+            return perf[f]
+
+    tests.sort(key=sortkey)
+
 class TestRunner(object):
     """Holds context for executing tests.
 
@@ -2335,44 +2379,7 @@ 
             os.umask(oldmask)
 
     def _run(self, testdescs):
-        if self.options.random:
-            random.shuffle(testdescs)
-        else:
-            # keywords for slow tests
-            slow = {b'svn': 10,
-                    b'cvs': 10,
-                    b'hghave': 10,
-                    b'largefiles-update': 10,
-                    b'run-tests': 10,
-                    b'corruption': 10,
-                    b'race': 10,
-                    b'i18n': 10,
-                    b'check': 100,
-                    b'gendoc': 100,
-                    b'contrib-perf': 200,
-                   }
-            perf = {}
-            def sortkey(f):
-                # run largest tests first, as they tend to take the longest
-                f = f['path']
-                try:
-                    return perf[f]
-                except KeyError:
-                    try:
-                        val = -os.stat(f).st_size
-                    except OSError as e:
-                        if e.errno != errno.ENOENT:
-                            raise
-                        perf[f] = -1e9 # file does not exist, tell early
-                        return -1e9
-                    for kw, mul in slow.items():
-                        if kw in f:
-                            val *= mul
-                    if f.endswith(b'.py'):
-                        val /= 10.0
-                    perf[f] = val / 1000.0
-                    return perf[f]
-            testdescs.sort(key=sortkey)
+        sorttests(testdescs, shuffle=self.options.random)
 
         self._testdir = osenvironb[b'TESTDIR'] = getattr(
             os, 'getcwdb', os.getcwd)()