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

Policy::names(bool) ignores its argument

    Details

    • Story Points:
      1
    • Epic Link:
    • Sprint:
      Alert Production S17 - 12
    • Team:
      Alert Production

      Description

      The method names(bool topLevelOnly = false) in pex::policy::Policy does not use its argument, always behaving as if topLevelOnly were true. It appears that this bug went undetected because the swig wrapper for names(list<string> &, bool, bool) masked names(bool), preventing it from ever being called from Python code.

      I propose that the implementation of names(bool) be changed to share code with names(list<string> &, bool, bool), both fixing the immediate problem and preventing inconsistent behavior from calling the wrong overload in the future.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Russell Owen, please review this fix.

            Show
            krzys Krzysztof Findeisen added a comment - Hi Russell Owen , please review this fix.
            Hide
            rowen Russell Owen added a comment -

            An excellent improvement to the code.

            My one minor discomfort is that the existing test for policyNames seems weak, and you changed the implementation of that method. However, the existing test may be good enough and your new implementation is a significant improvement (and almost certainly correct by inspection).

            I'm also a bit surprised you duplicated a test in C++ and Python. I realize that names(list, bool, bool) is not wrapped for Python and so much be tested in C++ , but why test names(bool) in both? However, I realize opinions may differ on this, and having written the C++ test it would be a waste to discard it.

            Show
            rowen Russell Owen added a comment - An excellent improvement to the code. My one minor discomfort is that the existing test for policyNames seems weak, and you changed the implementation of that method. However, the existing test may be good enough and your new implementation is a significant improvement (and almost certainly correct by inspection). I'm also a bit surprised you duplicated a test in C++ and Python. I realize that names(list, bool, bool) is not wrapped for Python and so much be tested in C++ , but why test names(bool) in both? However, I realize opinions may differ on this, and having written the C++ test it would be a waste to discard it.
            Hide
            krzys Krzysztof Findeisen added a comment -

            I guess my reasoning was that, given that I know there are/were problems with how the code was wrapped, it would be useful to know if the method behaves differently in one language than the other – information that could be used to diagnose future problems (indeed, prior to the implementation change, the C++ tests failed but the Python ones did not).

            Show
            krzys Krzysztof Findeisen added a comment - I guess my reasoning was that, given that I know there are/were problems with how the code was wrapped, it would be useful to know if the method behaves differently in one language than the other – information that could be used to diagnose future problems (indeed, prior to the implementation change, the C++ tests failed but the Python ones did not).

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                krzys Krzysztof Findeisen
                Reviewers:
                Russell Owen
                Watchers:
                Krzysztof Findeisen, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel