|
@rhl: What exactly do you mean by LSST-style wrapping of python's unittest? I have written python unittests for FiberTraces, Spectra, and PSF. Also, this issue is pretty much the same is INFRA-20? How should I name git branches for issues like INFRA-20 and PIPE2D-20? Standard I think is tickets/0020 for both of them...
|
|
As far as I can see, the total amount of test code added as part of this ticket is very small. However, it's a delta on top of a large amount of existing code in tests/FiberTraces.py. The issue description isn't really specific enough for me to understand whether the existing code + the new changes fulfil the goals. These changes do check that we extract the correct number of fibre traces from some predefined test data, and then verifies that their size lies within acceptable ranges. That seems like a good addition.
I have only minor comments on the new code:
- Please try not to add extra, unnecessary white space. (Line 44 of tests/FiberTraces.py reads " \n", which is a bit useless.)
- It would be good to specify why these particular values for the constants nFiberTraces, minLength and maxLength were chosen. The answer is presumably just that these correspond to the particular test data available in the repository, but, if so, say so.
- In general, the code in this file consists of a few very, very long methods. The new code just makes one of them longer. This is a really bad way to structure tests: much better to break them up into fine-grained chunks. I don't think restructuring the whole file is in scope for this ticket, but consider whether these new lines could be broken out into a separate method.
|