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

Add tests of dataIdContainer exceptions

    XMLWordPrintable

    Details

    • Story Points:
      0.25
    • Epic Link:
    • Sprint:
      AP F18-2
    • Team:
      Alert Production

      Description

      As part of Ian Sullivan's demo on using the new coverage system, we picked places lacking in coverage to add coverage to. I chose DataIdContainer in pipe_base/argumentParser.py, which didn't have a test suite dedicated to it, and had a lot of untested exception handling. This ticket is to clean that up.

        Attachments

          Issue Links

          relates to

          Improvement - An improvement or enhancement to an existing feature or task. DM-15162 Improve documentation for DataIdContainer

          • Done

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            Parejkoj John Parejko added a comment -

            Russell Owen: do you mind reviewing this small change? It adds a handful of tests of exception handling to DataIdContainer. There's a larger block of code that I'm not testing here because I don't understand how castDataIds is supposed to behave (somehow self.idList needs to get set to something ahead of time). A small improvement is better than nothing, anyway.

            Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28229/pipeline

            Show
            Parejkoj John Parejko added a comment - Russell Owen : do you mind reviewing this small change? It adds a handful of tests of exception handling to DataIdContainer. There's a larger block of code that I'm not testing here because I don't understand how castDataIds is supposed to behave (somehow self.idList needs to get set to something ahead of time). A small improvement is better than nothing, anyway. Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28229/pipeline
            Hide
            Permalink
            rowen Russell Owen added a comment -

            I'll have a look. For the record: dataIdList is a list of butler data IDs, which are simply dicts of key=value pairs. All values in the dicts start out as strings, because that is how they are read from the command line. Casting is used to convert keys to other types as needed (int is fairly common, for example "visit" should have an integer value). Clearly the documentation could use improvement.

            Show
            rowen Russell Owen added a comment - I'll have a look. For the record: dataIdList is a list of butler data IDs, which are simply dicts of key=value pairs. All values in the dicts start out as strings, because that is how they are read from the command line. Casting is used to convert keys to other types as needed (int is fairly common, for example "visit" should have an integer value). Clearly the documentation could use improvement.
            Hide
            Permalink
            rowen Russell Owen added a comment -

            Looks good. Thank you for doing this. I filed DM-15162 to improve the docs.

            I had one minor request on github and will add another here: please use unittest.mock (which is in the standard library) instead of mock (a 3rd party package).

            Show
            rowen Russell Owen added a comment - Looks good. Thank you for doing this. I filed DM-15162 to improve the docs. I had one minor request on github and will add another here: please use unittest.mock (which is in the standard library) instead of mock (a 3rd party package).
            Hide
            Permalink
            Parejkoj John Parejko added a comment -

            I'm confused: I'm using `unittest.mock`.

            Show
            Parejkoj John Parejko added a comment - I'm confused: I'm using `unittest.mock`.
            Hide
            Permalink
            rowen Russell Owen added a comment -

            I was confused. You're right. Sorry.

            Show
            rowen Russell Owen added a comment - I was confused. You're right. Sorry.
            Hide
            Permalink
            Parejkoj John Parejko added a comment -

            Thanks. Merged and Done.

            Show
            Parejkoj John Parejko added a comment - Thanks. Merged and Done.

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              Russell Owen
              Watchers:
              Ian Sullivan, John Parejko, John Swinbank, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.