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

Improve testing of copyPolymorphic

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: astshim
    • Story Points:
      0.5
    • Sprint:
      AP F18-2
    • Team:
      Alert Production

      Description

      While working on DM-11971 I found that CmpMap was missing an override of copyPolymorphic. Improve the unit tests so this would be caught.

      Consider removing the definition of copyPolymporphic and copy from Mapping, since that class is abstract. This may make it easier to detect omission in subclasses of Mapping, since such code will not compile. However, that still leaves deeper classes that could forget to override it, so unit testing is more important.

      Finally, remove the virtual since it is implied by override.

      Consider adding explicit @copydoc to copyPolymorphic, though Doxygen already shows the documentation.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            It is not possible to test copyPolymorphic directly but testing copy shows the error and is being done.

            I removed copyPolymorphic from Mapping, making it a virtual class. That required one trival change in functional.cc. I also removed redundant virtual keywords.

            Show
            rowen Russell Owen added a comment - - edited It is not possible to test copyPolymorphic directly but testing copy shows the error and is being done. I removed copyPolymorphic from Mapping , making it a virtual class. That required one trival change in functional.cc . I also removed redundant virtual keywords.
            Hide
            krzys Krzysztof Findeisen added a comment -

            It looks like you made no changes to the unit tests, including tests of copy. Was that intentional?

            Show
            krzys Krzysztof Findeisen added a comment - It looks like you made no changes to the unit tests, including tests of copy . Was that intentional?
            Hide
            rowen Russell Owen added a comment -

            Yes. I guess my previous comment was not clear enough. What I meant to say is that the existing tests of copy seem to be adequate to show the problem. So this turned into a more minor cleanup ticket.

            Show
            rowen Russell Owen added a comment - Yes. I guess my previous comment was not clear enough. What I meant to say is that the existing tests of copy seem to be adequate to show the problem. So this turned into a more minor cleanup ticket.
            Hide
            krzys Krzysztof Findeisen added a comment -

            In that case, I've no objections to merging.

            Show
            krzys Krzysztof Findeisen added a comment - In that case, I've no objections to merging.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Krzysztof Findeisen, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: