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: Fix some pytest problems #269

Merged
merged 5 commits into from Aug 8, 2017
Merged

DM-11514: Fix some pytest problems #269

merged 5 commits into from Aug 8, 2017

Conversation

timj
Copy link
Member

@timj timj commented Aug 8, 2017

  • Use correct name of pytest initialization function.
  • Rename a test file.
  • Cleanup logic in SConscript to ignore pybind11 test files.

@kfindeisen
Copy link
Member

I'm a bit surprised by the "Ignore..." and "Fix name of..." commits, because I've run pytest with no arguments from the command line with no problems. Is this an OS- or otherwise environment-specific issue?

@timj
Copy link
Member Author

timj commented Aug 8, 2017

@kfindeisen if you run pytest without arguments it will discover most of the tests but not the one test that was named incorrectly. Also, the ignore is for SCons' benefit so that it won't try to test that file. If you ask pytest to test a file without any tests in it, pytest returns non-zero exit status as it assumes you've made a mistake somewhere. If you ask pytest to run tests for tests/*.py then that one file without tests in it will be ignored on the basis that other tests were run okay. In the current implementation of sconsUtils we run pytest separately for each file so trigger this problem. Hiding the files from testing is the correct approach.

@kfindeisen
Copy link
Member

kfindeisen commented Aug 8, 2017

Ok, I didn't realize sconsUtils did its own file search.

And yes, I understand the problem with testDir.py. Sorry for mistaking it for a library!

@timj
Copy link
Member Author

timj commented Aug 8, 2017

Should have mentioned this PR is triggered by lsst/sconsUtils#34.

@timj
Copy link
Member Author

timj commented Aug 8, 2017

@kfindeisen does this count as a review?

@kfindeisen
Copy link
Member

No, I was asking for personal edification.

@timj timj requested a review from r-owen August 8, 2017 21:36
Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

I'm not entirely sure I understand the changes to stop testing for afwdir in tests/SConscript, but I assume they are correct (were you able to test them?).

One thing to consider: move the test in (formerly) testDir.py to test_sourceTable.py. The name of the test is a bit unclear and what it tests is an aspect of SourceTable. That said, it probably tests an aspect of all tables and so perhaps out to be rewritten to use SimpleTable. But I think simply moving it and deleting the old file would be good enough.

tests/SConscript Outdated
@@ -10,6 +10,7 @@ afwdataDir = env.ProductDir("afwdata")
if afwdataDir:
env["ENV"]["AFWDATA_DIR"] = afwdataDir

scripts.BasicSConscript.tests(noBuildList=[name + '.cc' for name in pybind11_test_modules])
scripts.BasicSConscript.tests(noBuildList=[name + '.cc' for name in pybind11_test_modules],
ignoreList=[name + '.py' for name in pybind11_test_modules])
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 curious: why is this change needed? I suppose if you use pytest tests/*.py instead of pytest, as we may have to do initially, that would explain it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed to stop SCons from thinking that the pybind11 .py files are things that should be tested. This is important when we are in a mode where pytest runs each test file separately because pytest exits with bad status if no tests are found. See my comment above replying to @kfindeisen. With these changes pytest without arguments would work but sconsUtils is not yet set up to do that (that's a change we can make later once the basics are done).

@timj
Copy link
Member Author

timj commented Aug 8, 2017

The tweaking of the {{afwdata}} stuff in the SCons file is a left over of DM-609. We don't need to filter out any tests because the C++ tests trap the situation themselves. I have tested with and without afwdata.

@timj timj merged commit b131724 into master Aug 8, 2017
@ktlim ktlim deleted the tickets/DM-11514-afw branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants