DM-11514: Use pytest as simple test runner #34

timj · 2017-08-07T21:38:16Z

This changes the behavior of sconsUtils testing:

Use pytest as the test runner, running a single process for all python tests, and multiple subprocesses if -j is used.
Allow some tests to be run standalone if they can't run with others.
Allow for test auto discovery, enabling flake8 testing of the entire product.
Send the output from pytest to both standard output and a file in tests/.tests (converting that to .failed as before).
Create a JUnit XML file for Jenkins to use.

Has not been needed for unittest in a long time and breaks pytest.

pytest will report the full path to the test directory and that might include spaces.

This runs each test file separately and writes a .xml JUnit file for each one.

r-owen

This looks like a very nice set of changes.

My only question is whether the pytest runner should start out by running on tests/*.py instead of using automatic test discovery, until we are sure the names of all our tests have been converted. But perhaps the RFC is further along than I realize.

r-owen · 2017-08-08T23:19:38Z

bin/makeLinkFarm

@@ -1,5 +1,6 @@
 #!/usr/bin/env python

+from __future__ import print_function


Please consider using our standard from __future__ import absolute_import, division, print_function. I admit division is unlikely in these files, but requiring absolute import is a useful addition, and it is the typical thing we add to all .py files.

r-owen · 2017-08-08T23:24:33Z

python/lsst/sconsUtils/builders.py

@@ -155,7 +155,7 @@ def filesToTag(root=None, fileRegex=None, ignoreDirs=None):
        #
        # Remove files generated by swig
        #
-        for swigFile in [f for f in filenames if re.search(r"\.i$", f)]:
+        for swigFile in [f for f in filenames if f.endswith(".i")]:


I'm sorry to see we still need this, but presumably removing SWIG support should be done on a different ticket.

The svn support can almost certainly be removed as well.

Not in this ticket.

jhoblitt · 2017-08-08T23:35:47Z

bin/makeLinkFarm

@@ -1,5 +1,6 @@
 #!/usr/bin/env python

+from __future__ import print_function
 import argparse, os, re, sys


Are multiple package imports on the same line OK?

No. Unless it's __future__.

Oh I see. I wasn't running flake8 on this file since it's not one we actually use. I'll fix it if you are insisting.

timj · 2017-08-08T23:36:50Z

I'm not going to touch the SWIG support as part of this cleanup. The reason I'm still using SCons for test detection (and why the ignoreList fixes have been needed downstream) is that this effectively means we have a one line change to switch to pytest and we don't have to tell people that the .failed files have disappeared or that per-file dependencies have gone.

Switching to running pytest without arguments would lead to a single JUnit XML file and one .failed file. We could do that with some bigger changes in sconsUtils to skip any .py files encountered as part of the test target and then add a single pytest if we found any. At the moment this wouldn't work until we renamed all the test files. Even so, the form you suggest would be one option if we want to go down that road now.

Deprecation warnings are easily lost so make them a bit more obvious in the test output. Verbose mode is not needed for Jenkins which will be using JUnit XML. It can be added by users by having a setup.cfg. If we want to allow use of $PYTEST_ADDOPTS we may have to pass that through the environment.

timj · 2017-08-11T19:50:20Z

@pschella now that I'm reading scripts.py line 443 I'm realising that we are doing the pybind11 test files in SConscript incorrectly. We should have added a pybind11NameList to the tests() method as is done for SWIG files. Then the test target could have automatically ignored the .py file and also ensured that the pybind11 files build before the the tests run. This would have avoided the noBuildList usage as well.

* A single call to pytest * Add new argument to enable auto test discovery * pytest always runs but uses --lf option * A single JUnit XML file is generated

Bug introduced in da6154c when "sources" was renamed to "objs".

When scons is run with -j (>1) pytest will now use pytest_xdist with -n argument.

timj · 2017-08-15T18:14:21Z

Good news. @ktlim, @r-owen, @RobertLuptonTheGood, if you now call scons -j N then pytest will run with N processes. I think this PR has now implemented everything that has been requested.

r-owen · 2017-08-15T23:08:41Z

Nice!

timj · 2017-08-16T04:27:52Z

daf_persistence fails with multi-process because test classes were sharing a temp directory. I've opened a ticket for that.

jhoblitt · 2017-08-22T17:17:28Z

python/lsst/sconsUtils/scripts.py

@@ -195,7 +195,16 @@ def finish(defaultTargets=DEFAULT_TARGETS,
                      nfail=`find {0} -name \*.failed | wc -l | sed -e 's/ //g'`; \
                      if [ $$nfail -gt 0 ]; then \
                          echo "Failed test output:" >&2; \
-                          find {0} -name \*.failed -exec cat {{}} \; >&2; \
+                          for f in `find {0} -name \*.failed`; do \


Why are the newline escapes needed inside of a heredoc?

You mean the bit after the do? Probably not needed. It was left over from the edited line.

No -- I meant why do any of the lines need escapes?

$ python Python 3.6.2 (default, Jul 19 2017, 13:09:21) [GCC 7.1.1 20170622 (Red Hat 7.1.1-3)] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import os >>> os.system(""" ... if [[ 1 == 1 ]]; then ... echo hi ... fi ... """) hi 0

No idea. Haven't checked. I was using the style from the pre-existing code. They all do turn up on a single line when you print the debug so I'm not sure why @RobertLuptonTheGood did it like that originally.

jhoblitt · 2017-08-22T17:21:42Z

python/lsst/sconsUtils/scripts.py

@@ -481,9 +507,31 @@ def s(l):
            swigMods.extend(
                state.env.SwigLoadableModule("_" + name, src, LIBS=state.env.getLibs("main python"))
            )
+
+        # Warn about insisting that a test in pySingles starts with test_ and therefore
+        # might be automatically discovered by pytest. This warning could only be issued


"should only"?

Don't know. Do you think it should only be issued if auto test discovery is enabled? I think I'd prefer the warning regardless so that we don't get hit with renaming the file again when auto test discovery is turned on. If we don't warn we are going to get problems later when people think they can just run pytest in the root directory.

I was attempting to call out that "could" either is in past tense or implies a level of uncertainty. It would be better to be definitive and say either "can only be" or "should only be".

Fair enough. I'll insist on it happening.

To answer the functional question, I'm in favor of always generating the warning to make sure the developer aware of the potential problem.

jhoblitt · 2017-08-22T20:24:36Z

python/lsst/sconsUtils/scripts.py

@@ -467,6 +491,8 @@ def tests(pyList=None, ccList=None, swigNameList=None, swigSrc=None,
            ignoreList = []

        def s(l):
+            if l is None:
+                return ['None']
            return [str(i) for i in l]


I know this is prior formatting but a newline would be nice here to visually separate the function block. I'm surprised that flake8 doesn't catch nested functions.

jhoblitt · 2017-08-22T20:37:37Z

python/lsst/sconsUtils/tests.py

@@ -127,18 +127,26 @@ def messages(self, test):
                    "false", "failed")

    def run(self, fileGlob):
+        """Create a test target for each file matching the supplied glob.
+        """


I don't object to this style but I think it is inconsistent with the rest of the package?

The documentation styles in sconsUtils are a disaster...

The other methods in that file use a similar style so I'm not sure what the problem is.

jhoblitt · 2017-08-22T20:40:10Z

python/lsst/sconsUtils/tests.py

+        pyList is a list of nodes corresponding to python test files.
+        The IgnoreList is respected when scanning for entries.
+        If pyList is None, we will use automated test discovery within
+        pytest. Returns a list containing a single target."""


What is the rule being used for line breaking?

you mean, for formatting the doc paragraph? You want a 72 character wide M-x fill-paragraph?

jhoblitt · 2017-08-22T20:52:24Z

tests/SConscript

@@ -5,7 +5,7 @@ import subprocess
 from lsst.sconsUtils import env, scripts, targets
 import SCons.Script

-scripts.BasicSConscript.tests()
+scripts.BasicSConscript.tests(pyList=[])


Doesn't this completely disable running tests?

That docstring you commented on above explains this. If you say pyList=[] that enables automatic test discovery. pyList=None invokes old style "find all *.py files". Using pyList like this was the simplest way to retain backwards compatibility and let you enable auto discovery. An entirely new argument to enable auto test discovery and disable pyList seemed overkill (I did try it that way first).

jhoblitt · 2017-08-22T21:03:26Z

python/lsst/sconsUtils/tests.py

+            echo "Global pytest run: failed"; \
+            mv ${{TARGET}}.out ${{TARGET}}.failed; \
+        fi;
+        """.format(interpreter, " ".join([pipes.quote(p) for p in pythonTestFiles]), libpathstr))


I just checked and pipes.quote is still present in py 3.6. The best option I could find for py2/3 compat is a backport module:

https://pypi.python.org/pypi/shellescape

which seems like a PITA for one small function. Perhaps add a comment that pipes.quote is deprecated and will probably break someday?

I had no idea that pipes.quote was deprecated. I'm happy to switch to shlex.quote next year when we drop python2... We use pipes.quote all over the place in sconsUtils so making a special note for this one is not going to be helpful.

(and we have to quote because my test system at home is sitting on a volume with a space in the name so everything breaks without doing that).

jhoblitt · 2017-08-22T21:04:11Z

python/lsst/sconsUtils/tools/cuda.py

@@ -19,15 +19,15 @@
 def CUDANVCCStaticObjectEmitter(target, source, env):
    tgt, src = SCons.Defaults.StaticObjectEmitter(target, source, env)
    for file in src:
-        lifile = os.path.splitext(src[0].rstr())[0] + '.linkinfo'
+        lifile = os.path.splitext(src[0].rstr())[0] + '.linkinfo'  # noqa


What checks are being disabled?

r-owen · 2017-08-23T15:12:24Z

.gitignore

@@ -1,3 +1,4 @@
+.cache


Will the cache file show up here (at the package root) when scons runs pytest? In the past I think it may have showed up in tests/.cache

it seems to in this case. This will ignore all directories named .cache so it will work either way.

r-owen · 2017-08-23T15:18:34Z

python/lsst/sconsUtils/eupsForScons.py

@@ -2,7 +2,7 @@
 import os

 try:
-    from eups import *
+    from eups import *  # noqa F403 F401 Importing everything into this namespace


I suppose this is so all eups symbols come in when this module is imported by other code?

If it's just for local usage it'd be better to change this.

yes, it's so that all the normal eups routines can be available instead of the fallbacks. Since we are explicitly specifying fallbacks then it doesn't seem entirely crazy to import an explicit list from eups but is it worth it?

Please at least expand on the comment a bit. If I was coding it I'd use a list, so the linter could detect undefined symbols. But I think the current code is defensible, especially if the list of all needed eups symbols is very large.

Now that I try it, I think the current situation is to be preferred. As soon as you import the EUPS versions, the linter complains about the redefinition of symbols later in the file and you have to # noqa those. The current approach has fewer noqa lines.

r-owen · 2017-08-23T15:25:32Z

python/lsst/sconsUtils/tests.py

+        """Add a single target for testing all python files. pyList is
+        a list of nodes corresponding to python test files. The
+        IgnoreList is respected when scanning for entries. If pyList
+        is None, we will use automated test discovery within pytest.


The condition for automatic test discovery is a bit more complicated than that. It is, at least: If pyList is None or an empty list... but if no tests are found in the provided nodes then it also goes into auto discovery mode, and that worries me a bit because auto discovery may be triggered unexpectedly.

If the current behavior (go into auto discovery if pythonTestFiles is empty) is desirable for some reason then please explain in much more detail in the doc string when this function may use auto test discovery.

If the current behavior is not needed, then please consider changing the code so that it tests pyList, not pythonTestFiles, to decide whether to use automatic discovery. And expanding the doc string only enough to state that [] and None will trigger it.

Ok. Yes. This is an internal function that does not distinguish [] from None (unlike the public interface).

Auto discovery can only be triggered unexpectedly if you are using the internal interface incorrectly. I didn't think it made sense to call runPythonTests and have it return immediately without running any tests.

With the EUPS product name and, optionally, an additional string taken from the `LSST_JUNIT_PREFIX` env var. This allows the test reports to be easily aggregated across multiple products that make up a build. Eg. tests.test_angle becomes either afw.tests.test_angle or afw.${LSST_JUNIT_PREFIX}.tests.test_angle

timj added 4 commits August 7, 2017 14:37

Add pytest EUPS dependency

2c6b210

Replace regex with startswith test

315568c

Modernize Python print and exception catching

2f45aaa

Remove trailing whitespace

0fefba4

timj force-pushed the tickets/DM-11514 branch from fd01bc4 to 0fefba4 Compare August 7, 2017 21:54

timj added 4 commits August 7, 2017 15:09

Ignore pytest cache

0701af8

Remove test runner cruft from tests

365ef76

Has not been needed for unittest in a long time and breaks pytest.

Rename testAll.py so that pytest can discover it itself (RFC-229)

d16253c

Tests should not be executable (RFC-215)

42618a0

timj force-pushed the tickets/DM-11514 branch from c8f4dd9 to 42618a0 Compare August 7, 2017 22:13

timj added 4 commits August 7, 2017 16:47

Quote the test path

2832d91

pytest will report the full path to the test directory and that might include spaces.

No longer need to import sys

b083808

Use pytest for test execution

9a44a87

This runs each test file separately and writes a .xml JUnit file for each one.

Replace re.search with endswith()

5ba5ce9

timj mentioned this pull request Aug 8, 2017

DM-11514: Fix some pytest problems lsst/afw#269

Merged

r-owen approved these changes Aug 8, 2017

View reviewed changes

jhoblitt reviewed Aug 8, 2017

View reviewed changes

timj changed the title ~~Use pytest as test runner~~ DM-11514: Use pytest as simple test runner Aug 9, 2017

Do not rewrite a shebang if the file does not start with a letter

8b18099

timj force-pushed the tickets/DM-11514 branch from f56ed88 to 8b18099 Compare August 9, 2017 19:10

Move a constant function out of loop

0759c19

timj force-pushed the tickets/DM-11514 branch from 60dc30f to 352124b Compare August 11, 2017 21:26

timj added 5 commits August 11, 2017 15:13

Call pytest once for all python test files

5d01e4d

* A single call to pytest * Add new argument to enable auto test discovery * pytest always runs but uses --lf option * A single JUnit XML file is generated

Use correct variable

032ddf4

Bug introduced in da6154c when "sources" was renamed to "objs".

Fix flake8 warnings

3bf8412

Enable automated test discovery

f7b7d39

Enable flake8 testing of source

88ce337

Enable multi-process testing

185a25c

When scons is run with -j (>1) pytest will now use pytest_xdist with -n argument.

This was referenced Aug 21, 2017

Force testCoadds to run in separate pytest process lsst/pipe_tasks#145

Closed

DM-11514: Support new pytest lsst/base#18

Merged

jhoblitt reviewed Aug 22, 2017

View reviewed changes

timj added 6 commits August 22, 2017 15:06

Indicate the failure code next to each noqa

44d74d5

Add a test for pySingles mode

e2044be

Add a blank line after inline def

e544b15

Reformat docstrings to meet 72 char limit

857b5d9

Use log.warn for warning message

d1d7c23

Correctly handle process vs processes

a4e6ccb

r-owen reviewed Aug 23, 2017

View reviewed changes

timj added 2 commits August 23, 2017 08:50

More explicit docs for runPythonTests

e37d2fa

Enhance the documentation in the sconsUtils EUPS package

1990145

timj force-pushed the tickets/DM-11514 branch from fb57bab to 1990145 Compare August 23, 2017 15:58

jhoblitt force-pushed the tickets/DM-11514 branch from 3bcc894 to e000e06 Compare August 24, 2017 22:22

timj merged commit e000e06 into master Aug 25, 2017

ktlim deleted the tickets/DM-11514 branch August 25, 2018 06:16

		@@ -1,5 +1,6 @@
		#!/usr/bin/env python

		from __future__ import print_function

DM-11514: Use pytest as simple test runner #34

DM-11514: Use pytest as simple test runner #34

Conversation

timj commented Aug 7, 2017 • edited

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj commented Aug 8, 2017

timj commented Aug 11, 2017

timj commented Aug 15, 2017

r-owen commented Aug 15, 2017

timj commented Aug 16, 2017

jhoblitt Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhoblitt Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj commented Aug 7, 2017 •

edited

jhoblitt Aug 22, 2017 •

edited

timj Aug 22, 2017 •

edited

jhoblitt Aug 22, 2017 •

edited

r-owen Aug 23, 2017 •

edited