Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-7248

Modify meas_algorithms tests to support pytest

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_algorithms
    • Labels:
      None
    • Team:
      Data Release Production

      Description

      This ticket is for the work of migrating the meas_algorithms tests such that they run with the py.test test runner.

        Attachments

          Issue Links

            Activity

            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Merged to master...

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Merged to master...
            Hide
            lauren Lauren MacArthur added a comment -

            I was just going to ask if this had been Jenkins'd!

            A few comments on the PR & there is one more place I think __file__ should be used: line 111 in testHtmIndex.py

            Also, please make sure all commit messages are concise and comply with the standards (also, some of Ian's changes look like fix ups to Vishal's...if it's not too painful, fixing this would be nice.

            Show
            lauren Lauren MacArthur added a comment - I was just going to ask if this had been Jenkins'd! A few comments on the PR & there is one more place I think __file__ should be used: line 111 in testHtmIndex.py Also, please make sure all commit messages are concise and comply with the standards (also, some of Ian's changes look like fix ups to Vishal's...if it's not too painful, fixing this would be nice.
            Hide
            rowen Russell Owen added a comment -

            I started a Jenkins py2 run with my changes: https://ci.lsst.codes/job/stack-os-matrix/compiler=gcc,label=centos-7,python=py2/14660//console
            which uses tickets/DM-7248 and origin/tickets/DM-7229 and explicitly includes lsst_ci

            Show
            rowen Russell Owen added a comment - I started a Jenkins py2 run with my changes: https://ci.lsst.codes/job/stack-os-matrix/compiler=gcc,label=centos-7,python=py2/14660//console which uses tickets/ DM-7248 and origin/tickets/ DM-7229 and explicitly includes lsst_ci
            Hide
            rowen Russell Owen added a comment - - edited

            I have pushed my changes

            Show
            rowen Russell Owen added a comment - - edited I have pushed my changes
            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Russell Owen If it will be easy for you to pull & rebase, please do so. Otherwise, let me know and I'll make the changes.

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Russell Owen If it will be easy for you to pull & rebase, please do so. Otherwise, let me know and I'll make the changes.
            Hide
            rowen Russell Owen added a comment -

            I also had a look. Overall this looks great to me. I do have a few suggested changes:

            Change "self.assertRaises" to "with self.assertRaises", at least in most cases (if a single function with no args is being called it may not enhance clarity enough to be worth changing, but in all other instances it enhances clarity):

            • testCoaddPsf.py
            • testGaussianPsfFactory.py
            • testHtmIndex.py
            • testLoadReferenceObjects.py
            • testMeasureApCorr.py
            • testPsfIO.py

            I also found a few flake8 warnings about variables not being set:

            • testMeasureApCorr line 139-ish apFluxName =... that line can be deleted
            • testPsfIO.py line 137-ish psf = ... get rid of psf

            One instance of commented-out code should probably be deleted:

            • testPsfIO.py: line399-ish # def assertClose...

            I actually made these changes already, but the branch got changed remotely while I did it. Not sure whether to pull and rebase or just let somebody else redo my work.

            Show
            rowen Russell Owen added a comment - I also had a look. Overall this looks great to me. I do have a few suggested changes: Change "self.assertRaises" to "with self.assertRaises", at least in most cases (if a single function with no args is being called it may not enhance clarity enough to be worth changing, but in all other instances it enhances clarity): testCoaddPsf.py testGaussianPsfFactory.py testHtmIndex.py testLoadReferenceObjects.py testMeasureApCorr.py testPsfIO.py I also found a few flake8 warnings about variables not being set: testMeasureApCorr line 139-ish apFluxName =... that line can be deleted testPsfIO.py line 137-ish psf = ... get rid of psf One instance of commented-out code should probably be deleted: testPsfIO.py: line399-ish # def assertClose... I actually made these changes already, but the branch got changed remotely while I did it. Not sure whether to pull and rebase or just let somebody else redo my work.
            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Hi Lauren,
            Can you please review the work done on this ticket?

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Hi Lauren, Can you please review the work done on this ticket?

              People

              • Assignee:
                vpk24 Vishal Kasliwal [X] (Inactive)
                Reporter:
                vpk24 Vishal Kasliwal [X] (Inactive)
                Reviewers:
                Lauren MacArthur
                Watchers:
                Ian Sullivan, Lauren MacArthur, Russell Owen, Vishal Kasliwal [X] (Inactive)
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel