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

Migrate sphgeom to pytest

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Make the unit tests in the sphgeom package compatible with pytest.

        Attachments

          Issue Links

            Activity

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

            Hi Maria,
            Can you please review the work done on this ticket? Thanks!

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Hi Maria, Can you please review the work done on this ticket? Thanks!
            Hide
            tjenness Tim Jenness added a comment -

            I have a quick comment, sphgeom does not currently depend on the utils package and it wasn't clear to me that we wanted to make it dependent on utils. It only depends on base and that means it can't ever trigger any of the memory testing citizen tracking warnings. This means the memory test is never going to trigger anything and I don't believe the file descriptor leak test is ever going to be triggered either. For this reason I'd rather we did not add a utils dependency to sphgeom so please remove all the utils boiler plate. i.e., we don't need the init calls, or setup_module or utils.TestCase. I realize that this is non-obvious given the documentation examples and adding a utils dependency might be a reasonable thing to do later if sphgeom gains extra dependencies.

            I did mention this to Pim Schellart [X] when he was working on the tests over the weekend.

            Show
            tjenness Tim Jenness added a comment - I have a quick comment, sphgeom does not currently depend on the utils package and it wasn't clear to me that we wanted to make it dependent on utils . It only depends on base and that means it can't ever trigger any of the memory testing citizen tracking warnings. This means the memory test is never going to trigger anything and I don't believe the file descriptor leak test is ever going to be triggered either. For this reason I'd rather we did not add a utils dependency to sphgeom so please remove all the utils boiler plate. i.e., we don't need the init calls, or setup_module or utils.TestCase . I realize that this is non-obvious given the documentation examples and adding a utils dependency might be a reasonable thing to do later if sphgeom gains extra dependencies. I did mention this to Pim Schellart [X] when he was working on the tests over the weekend.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Tim Jenness Yes, you are right. Unfortunately I forgot to relay this information to Vishal Kasliwal [X] when he started working on it instead. I think / hope it is not too much work to undo the new dependency.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Tim Jenness Yes, you are right. Unfortunately I forgot to relay this information to Vishal Kasliwal [X] when he started working on it instead. I think / hope it is not too much work to undo the new dependency.
            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Undoing all of that now. It shouldn't be a problem.

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Undoing all of that now. It shouldn't be a problem.
            Hide
            mtpatter Maria Patterson [X] (Inactive) added a comment -

            Ok, I added comments in the github conversation also - there are unused imports that I think can be removed. Also, if you want to remove the added #!/usr/bin/env python line, that might be better, since it was not there previously, (see https://jira.lsstcorp.org/browse/DM-3200 and https://jira.lsstcorp.org/browse/RFC-215). Otherwise, if those things are addressed and if it builds with Jenkins, looks good. Thanks. (And thanks also for my first review.)

            Show
            mtpatter Maria Patterson [X] (Inactive) added a comment - Ok, I added comments in the github conversation also - there are unused imports that I think can be removed. Also, if you want to remove the added #!/usr/bin/env python line, that might be better, since it was not there previously, (see https://jira.lsstcorp.org/browse/DM-3200 and https://jira.lsstcorp.org/browse/RFC-215 ). Otherwise, if those things are addressed and if it builds with Jenkins, looks good. Thanks. (And thanks also for my first review.)
            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Merged to master...

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

              People

              • Assignee:
                vpk24 Vishal Kasliwal [X] (Inactive)
                Reporter:
                pschella Pim Schellart [X] (Inactive)
                Reviewers:
                Maria Patterson [X] (Inactive)
                Watchers:
                Maria Patterson [X] (Inactive), Pim Schellart [X] (Inactive), Tim Jenness, Vishal Kasliwal [X] (Inactive)
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel