DM-11539: Fix test for pytest and make flake8 clean #90
Conversation
The measurement plugins registered by test_ApCorrNameSet can affect the tests in test_PluginLogs.py because they were not set up as real plugins. To allow test_PluginLogs to run within the same process as test_ApCorrNameSet, we now add the fail() and getExecutionOrder() methods to allow them to adhere to the plugin contract.
|
That's right. |
tests/test_PluginLogs.py
Outdated
| @@ -208,7 +208,7 @@ def testLoggingPythonPlugin(self): | |||
| pluginLogName = os.path.join(lsst.utils.getPackageDir('meas_base'), 'tests', 'plugin.log') | |||
| directLog(log, pluginLogName) | |||
| exposure, cat = self.dataset.realize(noise=0.0, schema=schema) | |||
| source = cat[0] | |||
| # source = cat[0] # noqa F841: Not sure if this is deliberate | |||
There was a problem hiding this comment.
Can we become sure, rather than just adding the note? If it doesn't break when you take it out, it looks very much like leaving it in was just an oversight.
There was a problem hiding this comment.
I have removed them as the test passes regardless. My comment was really for the reviewer.
tests/test_PluginLogs.py
Outdated
| @@ -235,7 +235,7 @@ def testLoggingCppPlugin(self): | |||
| pluginLogName = os.path.join(lsst.utils.getPackageDir('meas_base'), 'tests', 'plugin.log') | |||
| directLog(log, pluginLogName) | |||
| exposure, cat = self.dataset.realize(noise=0.0, schema=schema) | |||
| source = cat[0] | |||
| # source = cat[0] # noqa F841: Not sure if this is deliberate | |||
| if (self.measurement.doReplaceWithNoise and self.footprintDatasetName is not None | ||
| and self.references.removePatchOverlaps): | ||
| if (self.measurement.doReplaceWithNoise and self.footprintDatasetName is not None and | ||
| self.references.removePatchOverlaps): |
There was a problem hiding this comment.
I'm confused by this. I see that you're squelching a flake8 warning, but you actually seem to be going against the PEP8 recommendation (and against Knuth, which I personally think is much more significant). Is this a known issue in flake8? Has it been discussed elsewhere?
There was a problem hiding this comment.
It was W503. Weirdly, pycodestyle implies that W503 is disabled by default (this was changed in the past year). I am using the current version so it's almost like my flake8 is forcing it to be enabled. I've added it to the ignore list explicitly. I wasn't able to install the newest flake8 as an eups package because of some problem with setuptools version numbering.
| @@ -70,7 +70,7 @@ def testNaive(self): | |||
| position) | |||
| area = self.computeNaiveArea(position, radius) | |||
| # test that this isn't the same as the sinc flux | |||
| self.assertNotClose( | |||
| self.assertFloatsNotEqual( | |||
There was a problem hiding this comment.
Wasn't immediately clear why you did this — I had to go and look up the definition of assertNotClose to find out that it's deprecated. Maybe worth noting that in the commit message?
assertNotClose() is now deprecated.
timj
force-pushed
the
tickets/DM-11539
branch
2 times, most recently
from
c79e4d6
to
a5a16d0
Compare
Explicitly ignore W503. Some files have E266 ignored explicitly because of doxygen markup.
Ensures that the dummy classes registered in one test are still valid so that a later test will not complain about them.
Also make flake8 clean and fix a couple of warnings. The remaining flake8 warnings are caused by Doxygen
##syntax. @jonathansick I am assuming that they will be fixed as part of the migration to numpydoc.