-
Notifications
You must be signed in to change notification settings - Fork 3
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-13851: Speed up ap_verify unit tests #34
Changes from all commits
e42a788
d16045a
d1ddaf4
15efcf0
18784c1
fb829d1
901088c
f43d268
197f960
e46325b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
# -*- python -*- | ||
from lsst.sconsUtils import scripts | ||
scripts.BasicSConstruct("ap_verify") | ||
scripts.BasicSConstruct("ap_verify", disableCc=True) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
# | ||
# This file is part of ap_verify. | ||
# | ||
# Developed for the LSST Data Management System. | ||
# This product includes software developed by the LSST Project | ||
# (http://www.lsst.org). | ||
# See the COPYRIGHT file at the top-level directory of this distribution | ||
# for details of code ownership. | ||
# | ||
# This program is free software: you can redistribute it and/or modify | ||
# it under the terms of the GNU General Public License as published by | ||
# the Free Software Foundation, either version 3 of the License, or | ||
# (at your option) any later version. | ||
# | ||
# This program is distributed in the hope that it will be useful, | ||
# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
# GNU General Public License for more details. | ||
# | ||
# You should have received a copy of the GNU General Public License | ||
# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
# | ||
|
||
"""Common code for ap_verify unit tests. | ||
""" | ||
|
||
import unittest | ||
|
||
import lsst.utils.tests | ||
|
||
import lsst.pex.exceptions as pexExcept | ||
from lsst.ap.verify.config import Config | ||
|
||
|
||
class DataTestCase(lsst.utils.tests.TestCase): | ||
"""Unit test class for tests that need to use the Dataset framework. | ||
|
||
Unit tests based on this class will search for a designated dataset | ||
(`testDataset`), and skip all tests if the dataset is not available. | ||
|
||
Subclasses must call `DataTestCase.setUpClass()` if they override | ||
``setUpClass`` themselves. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Subclasses must call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EDIT: never mind; the encapsulation violations I was worried about are a consequence of how classes work in Python (specifically, the distinction between initialization and construction), not of |
||
""" | ||
|
||
testDataset = 'ap_verify_testdata' | ||
"""The EUPS package name of the dataset to use for testing (`str`). | ||
""" | ||
datasetKey = 'test' | ||
"""The ap_verify dataset name that would be used on the command line (`str`). | ||
""" | ||
|
||
@classmethod | ||
def setUpClass(cls): | ||
try: | ||
lsst.utils.getPackageDir(cls.testDataset) | ||
except pexExcept.NotFoundError: | ||
raise unittest.SkipTest(cls.testDataset + ' not set up') | ||
|
||
# Hack the config for testing purposes | ||
# Note that Config.instance is supposed to be immutable, so, depending on initialization order, | ||
# this modification may cause other tests to see inconsistent config values | ||
Config.instance._allInfo['datasets.' + cls.datasetKey] = cls.testDataset | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This worries me. Why do you have to do it this way? Can't you set the config values in an actual There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you saying that instead of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing that worries me is the "depending on initialization order... see inconsistent config values" bit. It also just feels hacky ("hack the config" afterall). I absolutely wouldn't advocate the "config object for testing convenience" suggestion you propose. That's definitely clumsy. Looking at it more, I thought |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have this derive from
TestCase
unless you want it to potentially be picked up as a test. Subclasses that are tests would then derive from this andTestCase
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? We used this pattern a lot in
afw
and didn't run into any problems. The fact that it's inpython/
(necessary to ensure it's on the path) should make it immune to that kind of bug.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the test classes themselves inherit from
DataTestCase
so I don't think there is a problem. The problem is when you put a class in a test file that looks like a class of tests but isn't really.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that pattern is in
afw
, I'd have pushed back on it in review.DataTestCase
is not aTestCase
, it's essentially a mixin for things that areTestCase
s.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree that it's a mixin (though maybe that word means something different in Python than in Java); it's a specific type of test case rather than extra functionality.
Anyway, I propose we revisit this later (and maybe the aforementioned
afw
test utilities at the same time), since this bit of code sharing is not relevant to the actual speed-up work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Maybe I should make a Community post to discuss it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe discuss with Russell first, because
lsst.afw.geom.testUtils.TransformTestBaseClass
(the "lots" of uses inafw
I thought I remembered 😰) was his idea.