Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 36 commits into from
Aug 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
2c6b210
Add pytest EUPS dependency
timj Aug 7, 2017
315568c
Replace regex with startswith test
timj Aug 7, 2017
2f45aaa
Modernize Python print and exception catching
timj Aug 7, 2017
0fefba4
Remove trailing whitespace
timj Aug 7, 2017
0701af8
Ignore pytest cache
timj Aug 7, 2017
365ef76
Remove test runner cruft from tests
timj Aug 7, 2017
d16253c
Rename testAll.py so that pytest can discover it itself (RFC-229)
timj Aug 7, 2017
42618a0
Tests should not be executable (RFC-215)
timj Aug 7, 2017
2832d91
Quote the test path
timj Aug 7, 2017
b083808
No longer need to import sys
timj Aug 7, 2017
9a44a87
Use pytest for test execution
timj Aug 8, 2017
5ba5ce9
Replace re.search with endswith()
timj Aug 8, 2017
8b18099
Do not rewrite a shebang if the file does not start with a letter
timj Aug 9, 2017
f4d0e2f
Enable deprecation warnings and disable verbose mode
timj Aug 11, 2017
0759c19
Move a constant function out of loop
timj Aug 11, 2017
5d01e4d
Call pytest once for all python test files
timj Aug 11, 2017
032ddf4
Use correct variable
timj Aug 11, 2017
3bf8412
Fix flake8 warnings
timj Aug 11, 2017
f7b7d39
Enable automated test discovery
timj Aug 11, 2017
88ce337
Enable flake8 testing of source
timj Aug 11, 2017
415bdd6
Add support for python tests that must be run in their own process
timj Aug 11, 2017
68ad088
Quote filenames sent to shell
timj Aug 11, 2017
e48effd
Add a test that we know will pass
timj Aug 14, 2017
f5d07ad
Preserve existing .failed behavior
timj Aug 14, 2017
cb92d30
Trap case where there are no test files to be found
timj Aug 15, 2017
b85720c
Depend on pytest_xdist
timj Aug 15, 2017
185a25c
Enable multi-process testing
timj Aug 15, 2017
44d74d5
Indicate the failure code next to each noqa
timj Aug 22, 2017
e2044be
Add a test for pySingles mode
timj Aug 22, 2017
e544b15
Add a blank line after inline def
timj Aug 22, 2017
857b5d9
Reformat docstrings to meet 72 char limit
timj Aug 22, 2017
d1d7c23
Use log.warn for warning message
timj Aug 22, 2017
a4e6ccb
Correctly handle process vs processes
timj Aug 22, 2017
e37d2fa
More explicit docs for runPythonTests
timj Aug 23, 2017
1990145
Enhance the documentation in the sconsUtils EUPS package
timj Aug 23, 2017
e000e06
prefix junit class names
jhoblitt Aug 24, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.sconsign.dblite
config.log
.sconf_temp
Expand Down
29 changes: 17 additions & 12 deletions bin/makeLinkFarm
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
#!/usr/bin/env python

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


def linkFiles(dstDir, srcDir, verbose=0, force=False):
"""Link all the regular files in srcDir into dstDir; mkdir all the directories in srcDir and
descend recursively"""
Expand All @@ -18,17 +22,18 @@ def linkFiles(dstDir, srcDir, verbose=0, force=False):
linkFiles(fullDst, fullSrc, verbose, force)
else:
if verbose > 1:
print "%s -> %s" % (fullSrc, fullDst)
print("%s -> %s" % (fullSrc, fullDst))

if os.path.exists(fullDst):
if force:
os.unlink(fullDst)
else:
print >> sys.stderr, "%s already exists; use --force to overwrite" % fullDst
print("%s already exists; use --force to overwrite" % fullDst, file=sys.stderr)
sys.exit(1)

os.symlink(fullSrc, fullDst)


def main():
parser = argparse.ArgumentParser(description='''Build a link farm to replace long -I/-L argument lists.
All the bin, include files, and libraries for all the dependencies of the specified product are linked into
Expand All @@ -44,13 +49,13 @@ creates a link farm for your current selection of products.

parser.add_argument('--force', '-f', action='store_true', help="Force creation of farm even if it exists")
parser.add_argument('--verbose', '-v', action='count', help="Be chatty")

args = parser.parse_args()

linkFarmDir = os.path.abspath(args.linkFarmDir)

if not args.force and os.path.exists(linkFarmDir):
print >> sys.stderr, "Path %s already exists; use --force to overwrite" % args.linkFarmDir
print("Path %s already exists; use --force to overwrite" % args.linkFarmDir, file=sys.stderr)
sys.exit(1)

#
Expand All @@ -64,23 +69,23 @@ creates a link farm for your current selection of products.
products = {}
for productName, versionName, optional, recursionDepth in \
eups.getDependencies(args.productName, None, setup=True):
if products.has_key(productName):
if productName in products:
continue

products[productName] = 1

pd = eups.productDir(productName)
if not pd: # no product
continue

binDir = os.path.join(pd, "bin")
incDir = os.path.join(pd, "include")
libDir = os.path.join(pd, "lib")

if args.verbose:
if args.verbose > 1 or \
os.path.exists(binDir) or os.path.exists(incDir) or os.path.exists(libDir):
print "%-15s %s" % (productName, pd)
print("%-15s %s" % (productName, pd))

if os.path.exists(binDir):
fullDstDir = os.path.join(linkFarmDir, "bin")
Expand All @@ -92,22 +97,22 @@ creates a link farm for your current selection of products.

if os.path.exists(libDir):
for src in os.listdir(libDir):
if not re.search(r"^lib", src):
if not src.startswith("lib"):
continue

fullSrc = os.path.join(libDir, src)
fullDst = os.path.join(linkFarmDir, "lib", src)
if args.verbose > 1:
print src, fullSrc, fullDst
print(src, fullSrc, fullDst)
if os.path.exists(fullDst):
if args.force:
os.unlink(fullDst)
else:
print >> sys.stderr, "%s already exists; use --force to overwrite" % fullDst
print("%s already exists; use --force to overwrite" % fullDst, file=sys.stderr)
sys.exit(1)

os.symlink(fullSrc, fullDst)


if __name__ == "__main__":
main()

34 changes: 18 additions & 16 deletions bin/sconsOpts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
#
# Print the options last passed to scons
#
import os, sys
from __future__ import print_function
import os
import sys
import ConfigParser
import argparse

parser = argparse.ArgumentParser(description="Tell us how scons was invoked")
parser.add_argument('configFile', type=str, nargs="?", default=None, help='The file with the information we need')
parser.add_argument('configFile', type=str, nargs="?", default=None,
help='The file with the information we need')
parser.add_argument('--cc', type=str, default="gcc",
help="Use this compiler if build.cfg is unavailable")
parser.add_argument('--opt', type=int, default=0,
Expand All @@ -17,28 +20,27 @@ parser.add_argument('--quiet', '-q', action="store_true", help="Don't generate a
args = parser.parse_args()
dirName = "."
if args.configFile and os.path.isdir(args.configFile):
dirName, args.configFile = args.configFile, None
dirName, args.configFile = args.configFile, None

if not args.configFile:
args.configFile = os.path.join(dirName, ".sconf_temp", "build.cfg")
args.configFile = os.path.join(dirName, ".sconf_temp", "build.cfg")

cc = args.cc
opt = args.opt

if os.path.exists(args.configFile):
config = ConfigParser.ConfigParser()
try:
config.read(args.configFile)
config = ConfigParser.ConfigParser()
try:
config.read(args.configFile)

cc = config.get("Build", 'cc')
opt = config.get("Build", 'opt', 'opt')
except Exception, e:
if not args.quiet:
print >> sys.stderr, "File %s: error %s" % (args.configFile, e)
cc = config.get("Build", 'cc')
opt = config.get("Build", 'opt', 'opt')
except Exception as e:
if not args.quiet:
print("File %s: error %s" % (args.configFile, e), file=sys.stderr)

else:
if not args.quiet:
print >> sys.stderr, "File %s doesn't exist" % args.configFile
if not args.quiet:
print("File %s doesn't exist" % args.configFile, file=sys.stderr)

print "cc=%s opt=%s" % (cc, opt)

print("cc=%s opt=%s" % (cc, opt))
4 changes: 2 additions & 2 deletions python/lsst/sconsUtils/builders.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def SourcesForSharedLibrary(self, files):
obj = self.SharedObject(ccFile)
objs.append(obj)

objs = sorted(state.env.Flatten(sources), key=str)
objs = sorted(state.env.Flatten(objs), key=str)
return objs


Expand Down Expand Up @@ -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")]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The svn support can almost certainly be removed as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this ticket.

name = os.path.splitext(swigFile)[0]
candidates = [f for f in candidates if not re.search(r"%s(_wrap\.cc?|\.py)$" % name, f)]

Expand Down
2 changes: 1 addition & 1 deletion python/lsst/sconsUtils/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -587,4 +587,4 @@ def getLibs(env, categories="main"):

SConsEnvironment.getLibs = getLibs

## @}
## @} # noqa E266
5 changes: 4 additions & 1 deletion python/lsst/sconsUtils/eupsForScons.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
import os

try:
from eups import *
# Prefer to use native EUPS but if that is not available, fallback
# versions are defined. Only a subset of EUPS functions are required
# but all are imported to prevent warnings from redefinitions below.
from eups import * # noqa F403 F401
eupsLoaded = True
except ImportError:
eupsLoaded = False
Expand Down
2 changes: 1 addition & 1 deletion python/lsst/sconsUtils/installation.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ def InstallEups(env, dest, files=[], presetup=""):

path = eups.Eups.setEupsPath()
if path:
locks = eups.lock.takeLocks("setup", path, eups.lock.LOCK_SH)
locks = eups.lock.takeLocks("setup", path, eups.lock.LOCK_SH) # noqa F841 keep locks active
env["ENV"]["EUPS_LOCK_PID"] = os.environ.get("EUPS_LOCK_PID", "-1")
except ImportError:
state.log.warn("Unable to import eups; not locking")
Expand Down
61 changes: 55 additions & 6 deletions python/lsst/sconsUtils/scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Copy link
Member

@jhoblitt jhoblitt Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the newline escapes needed inside of a heredoc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@timj timj Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case "$$f" in \
*.xml.failed) \
echo "Global pytest output is in $$f" >&2; \
;; \
*.failed) \
cat $$f >&2; \
;; \
esac; \
done; \
echo "The following tests failed:" >&2;\
find {0} -name \*.failed >&2; \
echo "$$nfail tests failed" >&2; exit 1; \
Expand Down Expand Up @@ -299,8 +308,10 @@ def rewrite_shebang(target, source, env):
if src is None:
src = Glob("#bin.src/*")
for s in src:
if str(s) != "SConscript":
result = state.env.Command(target=os.path.join(Dir("#bin").abspath, str(s)),
filename = str(s)
# Do not try to rewrite files starting with non-letters
if filename != "SConscript" and re.match("[A-Za-z]", filename):
result = state.env.Command(target=os.path.join(Dir("#bin").abspath, filename),
source=s, action=rewrite_shebang)
state.targets["shebang"].extend(result)

Expand Down Expand Up @@ -424,6 +435,13 @@ def doc(config="doxygen.conf.in", projectName=None, projectNumber=None, **kw):
# @param pyList A sequence of Python tests to run (including .py extensions).
# Defaults to a *.py glob of the tests directory, minus any
# files corresponding to the SWIG modules in swigFileList.
# An empty list will enable automated test discovery.
# @param pySingles A sequence of Python tests to run (including .py extensions)
# as independent single tests. By default this list is empty
# and all tests are run in a single pytest call.
# Items specified here will not appear in the default pyList
# and should not start with "test_" (such that they will not
# be auto-discoverable by pytest).
# @param ccList A sequence of C++ unit tests to run (including .cc extensions).
# Defaults to a *.cc glob of the tests directory, minus any
# files that end with *_wrap.cc and files present in swigSrc.
Expand All @@ -439,10 +457,12 @@ def doc(config="doxygen.conf.in", projectName=None, projectNumber=None, **kw):
##
@staticmethod
def tests(pyList=None, ccList=None, swigNameList=None, swigSrc=None,
ignoreList=None, noBuildList=None,
ignoreList=None, noBuildList=None, pySingles=None,
args=None):
if noBuildList is None:
noBuildList = []
if pySingles is None:
pySingles = []
if swigNameList is None:
swigFileList = Glob("*.i")
swigNameList = [_getFileBase(node) for node in swigFileList]
Expand All @@ -459,6 +479,10 @@ def tests(pyList=None, ccList=None, swigNameList=None, swigSrc=None,
pyList = [node for node in Glob("*.py")
if _getFileBase(node) not in swigNameList and
os.path.basename(str(node)) not in noBuildList]
# if we got no matches, reset to None so we do not enabled
# auto test detection in pytest
if not pyList:
pyList = None
if ccList is None:
ccList = [node for node in Glob("*.cc")
if (not str(node).endswith("_wrap.cc")) and str(node) not in allSwigSrc and
Expand All @@ -467,7 +491,10 @@ 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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


state.log.info("SWIG modules for tests: %s" % s(swigFileList))
state.log.info("Python tests: %s" % s(pyList))
state.log.info("C++ tests: %s" % s(ccList))
Expand All @@ -481,9 +508,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. These files
# should not be discovered automatically.
for node in pySingles:
if str(node).startswith("test_"):
state.log.warn("Warning: {} should be run independently but"
" can be automatically discovered".format(node))

# Ensure that python tests listed in pySingles are not included in pyList.
if pyList is not None:
pyList = [str(node) for node in pyList if str(node) not in pySingles]

ccList = [control.run(str(node)) for node in ccList]
pyList = [control.run(str(node)) for node in pyList]
pySingles = [control.run(str(node)) for node in pySingles]

# If we tried to discover .py files and found none, do not then
# try to use auto test discovery.
if pyList is not None:
pyList = [control.runPythonTests(pyList)]
else:
pyList = []
pyList.extend(pySingles)
for pyTest in pyList:
state.env.Depends(pyTest, ccList)
state.env.Depends(pyTest, swigMods)
state.env.Depends(pyTest, state.targets["python"])
state.env.Depends(pyTest, state.targets["shebang"])
Expand Down Expand Up @@ -533,4 +582,4 @@ def examples(ccList=None, swigNameList=None, swigSrc=None):
state.targets["examples"].extend(results)
return results

## @}
## @} # noqa E266