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

Some AFW tests are not enabled with no explanation

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Story Points:
      2
    • Epic Link:
    • Sprint:
      Science Pipelines DM-S15-4, Science Pipelines DM-S15-5
    • Team:
      Alert Production

      Description

      Running coverage.py on the AFW test suite indicated that two test classes in tests/wcs1.py are disabled. WCSTestCaseCFHT was added by Robert Lupton in 2007 but disabled during a merge a long time ago by Jim Bosch in 2010 but with no indication as to why. WCSRotateFlip appeared in 2012 (added by Simon Krughoff) but doesn't appear in the suite list at the end and so does not execute.

      Similarly testSchema.py has two tests that are not run: xtestSchema and testJoin. I assume xtestSchema is deliberately disabled but could there at least be a comment in the test explaining why?

      My feeling is that we should either run the tests or they should be removed. Having them their gives the impression they are doing something useful.

      Less importantly, warpExposure.py has some support code for comparing masked images that was written in 2009 by Russell Owen but which is not used anywhere in the test.

        Attachments

          Activity

          Ascending order - Click to sort in descending order
          Hide
          Permalink
          jbosch Jim Bosch added a comment -

          The tests in testSchema.py are not intentionally disabled and should be enabled:

          • I'm pretty sure testSchema() was disabled temporarily and accidentally not re-enabled before merging to master (which should have been caught in code review, but wasn't for some reason).
          • testJoin is not being run because there are actually two functions with that name, and one is shadowing the other. We should just rename one of them.

          WCSTestCaseCFHT was pretty bitrotted, but I've managed to fix it.

          I've put both of these changes on branch u/jbosch/DM-2929.

          Show
          jbosch Jim Bosch added a comment - The tests in testSchema.py are not intentionally disabled and should be enabled: I'm pretty sure testSchema() was disabled temporarily and accidentally not re-enabled before merging to master (which should have been caught in code review, but wasn't for some reason). testJoin is not being run because there are actually two functions with that name, and one is shadowing the other. We should just rename one of them. WCSTestCaseCFHT was pretty bitrotted, but I've managed to fix it. I've put both of these changes on branch u/jbosch/ DM-2929 .
          Hide
          Permalink
          rowen Russell Owen added a comment -

          The code in warpExposure.py that compares masked images was probably used before I centralized that code in afw.testUtils. There is certainly no point in keeping it around.

          Regarding one unit test shadowing another: unfortunately this seems to be a common mistake. Python linters report it (at least pyflakes does), and that is typically how I discover it.

          Show
          rowen Russell Owen added a comment - The code in warpExposure.py that compares masked images was probably used before I centralized that code in afw.testUtils. There is certainly no point in keeping it around. Regarding one unit test shadowing another: unfortunately this seems to be a common mistake. Python linters report it (at least pyflakes does), and that is typically how I discover it.
          Hide
          Permalink
          krughoff Simon Krughoff added a comment -

          Turns out this is a bit of a bigger issue than I thought. WCSRotateFlip does not pass, and it shouldn't according to the numbers I'm looking at. Hoped it would be a 1 hour thing, but it's going to take a bit more than that. Everything else mentioned here seems straightforward.

          Show
          krughoff Simon Krughoff added a comment - Turns out this is a bit of a bigger issue than I thought. WCSRotateFlip does not pass, and it shouldn't according to the numbers I'm looking at. Hoped it would be a 1 hour thing, but it's going to take a bit more than that. Everything else mentioned here seems straightforward.
          Hide
          Permalink
          krughoff Simon Krughoff added a comment -

          This is only in afw. There is a small change in Wcs.cc, other than that, all the rest of the changes are in tests.

          Show
          krughoff Simon Krughoff added a comment - This is only in afw. There is a small change in Wcs.cc, other than that, all the rest of the changes are in tests.
          Hide
          Permalink
          jbosch Jim Bosch added a comment -

          Looks great. Only comment (on the PR) is the tiniest of all possible comments.

          Show
          jbosch Jim Bosch added a comment - Looks great. Only comment (on the PR) is the tiniest of all possible comments.
          Hide
          Permalink
          krughoff Simon Krughoff added a comment -

          Thanks Jim. This is now merged.

          Show
          krughoff Simon Krughoff added a comment - Thanks Jim. This is now merged.

            People

            Assignee:
            krughoff Simon Krughoff
            Reporter:
            tjenness Tim Jenness
            Reviewers:
            Jim Bosch
            Watchers:
            Jim Bosch, John Swinbank, Robert Lupton, Russell Owen, Simon Krughoff, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: