DM-34133: Add ScatterPlotWithTwoHistsTaskTestCase #30
Conversation
taranu
force-pushed
the
tickets/DM-34133
branch
from
1567f02
to
74ddbc4
Compare
| @@ -57,6 +57,35 @@ class ScatterPlotWithTwoHistsTaskConfig(pipeBase.PipelineTaskConfig, | |||
| itemtype=str | |||
| ) | |||
|
|
|||
| def get_requirements(self): | |||
There was a problem hiding this comment.
This should have an underscore in the beginning, since this is not meant to be a public API (or is it?). I don't have a good alternative, but requirements seems strange. _get_bands_and_columns, may be?
There was a problem hiding this comment.
I don't see any need to make it quasi-private - it's purely a getter that doesn't change state - and while requirements isn't a great name, it's meant to specify that these are the things that the config requires for the task to run.
tests/test_scatterPlot.py
Outdated
| n = len(flux) | ||
| self.bands, columns = config.get_requirements() | ||
| data = { | ||
| 'refcat_flux': flux, |
There was a problem hiding this comment.
There are quite a few single quotes in this file.
There was a problem hiding this comment.
I'll change them for consistency, but this is not actually a PEP-8 or DM requirement.
tests/test_scatterPlot.py
Outdated
| filename_figure_tmp = os.path.join(self.testDir, "test_scatterPlot.png") | ||
| result.scatterPlot.savefig(filename_figure_tmp) | ||
| diff = compare_images(filename_figure_tmp, filename_figure_ref, 0) | ||
| self.assertIsNone(diff) |
There was a problem hiding this comment.
This is likely not going to give a helpful error message.
There was a problem hiding this comment.
This is the correct way to check/raise: https://github.com/LSSTDESC/skyproj/blob/main/tests/test_plotting.py#L47-L48
I'm sure the tolerance will have to be >0 because there are small differences on different platforms.
There was a problem hiding this comment.
Well, it passed Jenkins with 0 tolerance.
compare_images returns a human-readable message on failure, and pytest prints this out if the assert fails, so what's the benefit of raising an exception in a test?
There was a problem hiding this comment.
Did it print with the \ns like that? Because if you do the matplotlib raise then it'll print the message correctly formatted.
There was a problem hiding this comment.
It was verbatim, with the \ns like that.
There was a problem hiding this comment.
Ok, raise it is then.
| del self.data | ||
| del self.task | ||
|
|
||
| def test_ScatterPlotWithTwoHistsTask(self): |
There was a problem hiding this comment.
You also need to protect plotting tests with the default matplotlibrc:
https://github.com/LSSTDESC/skyproj/blob/702ac340e46cccae37ad91194d017816bf596ceb/tests/test_plotting.py#L17
|
|
||
| import unittest | ||
| import lsst.utils.tests | ||
|
|
There was a problem hiding this comment.
For testing with matplotlib, you need this at the top: https://github.com/LSSTDESC/skyproj/blob/702ac340e46cccae37ad91194d017816bf596ceb/tests/test_plotting.py#L4
taranu
force-pushed
the
tickets/DM-34133
branch
from
74ddbc4
to
a0f8528
Compare
taranu
force-pushed
the
tickets/DM-34133
branch
from
a0f8528
to
6ffb152
Compare
No description provided.