git blame reveals that this is your doing. Could you fix it?

I see that there's a similar problem in testWcsFitsTable.py that should be fixed at the same time. I didn't do an exhaustive audit, but there may be other cases too.

I also noticed that test.fits wasn't cleaned up if the tests failed. Is this deliberate? If so, there should probably be a comment or a note printed to stdout that tells the user that the file has been left behind for debugging.

This should be a simple review. Just some name changing to insure uniqueness. Not sure if you are setup yet to do a review, but I assume so, since you are on the list of reviewers.

The test framework (I am told) only runs an individual .py file in the test directory only once at any given time (never concurrently), so the name uniqueness just has to be good enough to not conflict with any other test. Since the files are generally stored beneath the package/tests directory (in this case afw/tests), only other tests in that directory have a potential to conflict.

Changes look fine to me; only comment would be that you should probably squash the two commits together using git rebase -i before merging, as there's no need for the second commit to be preserved in the history as separate from the first.

John Swinbank, the usual procedure for responding to a review would be to comment here, then transition the ticket to "Review Complete" when you're satisfied (either when the code is in all fixed, or when your comments were trivial enough that you don't feel you need to see it again after it's fixed). I'll let you do that here.

I agree with Jim that these commits should be squashed before merging.

What should the failure mode be if testDM384.fits already exists? Will catalog.writeFits() clobber it, or will the test fail? If the former, is that desired behaviour?

If the test fails for another reason, do you want to clean up testDM384.fits or leave it lying around for forensics?

I'm not sure if this is within the scope of this review, but: structurally, this looks more like three tests than one to me. That's ugly, and there's a lot of repeated code. You could consider restructuring things to avoid the repetition and make the cleanup automatic. Something like:

class TestDM384(unittest.TestCase):

    def setUp(self):

        self.filename = "testDM384.fits"

        self.schema = lsst.afw.table.Schema()

        self.schema.setVersion(5)

        self.schema.addField("f1", doc="f1a", type="I")

        self.schema.addField("f2", doc="f2a", type="Flag")

        self.schema.addField("f3", doc="f3a", type="ArrayF", size=4)

    def tearDown(self):

        os.unlink(self.filename)

    def _run_test(self, catalog):

        self.assertEqual(catalog.getTable().getVersion(), 5)

        catalog.writeFits(self.filename)

        # now read the table just written to disk, and see if it reads back correctly

        catalog = catalog.readFits(self.filename)

        metadata = catalog.getTable().getMetadata()

        self.assertEqual(catalog.schema.getVersion(), 5)

        self.assertFalse(metadata == None)

        self.assertFalse(metadata.exists("AFW_TABLE_VERSION"))

    def test_BaseCatalog(self):

        self._run_test(BaseCatalog(self.schema))

    def test_SimpleCatalog(self):

        self._run_test(SimpleCatalog(self.schema))

    def test_SourceCatalog(self):

        self._run_test(SourceCatalog(self.schema))

(I've not actually tested the above, so it's probably riddled with typos etc, and I'm not sure if there's a substantive difference between lsst.afw.table.SimpleTable.makeMinimalSchema and lsst.afw.table.Schema, but you get the gist.)

This goes significantly beyond the immediate scope of this issue, though, so I don't think it has to be done before this is merged. (Indeed, I'm not even sure if I'm allowed to request it here, or if I should open a new issue...)

I think it's normally best to put requests for large changes that are beyond the scope of the original issue on a new issue (small change requests are fine). And it's also allowed for reviewees to take what they feel is a too-large change request and make a new issue instead of addressing it immediately. I'll leave it to Perry to decide whether to do your suggested cleanup here, or push it to a new issue, as I think it falls somewhere in the middle.

And since Perry might not know the answer off the top of his head, I can confirm that writeFits will clobber, and I do think that's the desired behavior. I personally like leaving the files around for forensics after a test failure, but I'm not certain that opinion is unanimous.

I queried Robin on whether finally statements should be included in tests which create files, and her response is that leaving the test files around on failure is desirable.

This isn't my test, and the only thing I did when I read it was to look for name with test.fits and remove them. However, I agree that using the same filename three times is a bad idea, even though it is actually OK to destroy each file once the test has passed. But the lack of unique names is confusing. So I will fix that, but not restructure the code.

If this is OK with you, please mark Review Complete so that it will come back to me.

Ok; agreed that further restructuring is outside the narrow scope of this issue.