Opened 8 years ago

Last modified 7 years ago

#6885 defect new

MultiService.removeService behaves unexpectedly when given a named service that is not a child

Reported by: Jeremy Thurgood Owned by: Jeremy Thurgood
Priority: low Milestone:
Component: core Keywords:
Cc: Branch: branches/MultiService-remove-named-nonchild-fix-6885
branch-diff, diff-cov, branch-cov, buildbot
Author: jerith

Description

MultiService.removeService() checks for a service name and removes the service from its namedServices dict before attempting to remove the service from its services list.

If the parent has no child with the same name as the non-child service it is being asked to remove, a KeyError is raised instead of the documented ValueError.

If the parent has a child with the same name as the non-child service it is being asked to remove, that child is removed from the namedServices dict before the operation fails.

Attachments (3)

MultiService_remove_named_nonchild_fix.diff (2.5 KB) - added by Jeremy Thurgood 8 years ago.
MultiService_remove_named_nonchild_fix.2.diff (2.5 KB) - added by Jeremy Thurgood 8 years ago.
MultiService-remove-named-nonchild-fix-6885.diff (2.9 KB) - added by Jeremy Thurgood 8 years ago.
With the NEWS entry this time.

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by Jeremy Thurgood

Changed 8 years ago by Jeremy Thurgood

Changed 8 years ago by Jeremy Thurgood

With the NEWS entry this time.

comment:1 Changed 8 years ago by Jeremy Thurgood

Please ignore the first two diffs. I forgot about the NEWS entry and then I forgot how to operate svn. :-/

comment:2 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jeremy Thurgood

Thanks.

  1. The test methods should be split up so they each exercise only one case or feature. A weak rule of thumb to use is that there should only be one assertXXX call per test method.
  2. The test methods should all have docstrings.
  3. There are some style issues:
    1. Two blank lines between methods
    2. Use more expressive variable names
    3. Please use some more specific language than "more cleanly" in the news fragment

I considered whether changing the behavior of MultiService.removeService to match the interface documentation is better or worse than doing the reverse. Considering IServiceCollection.removeService does actually document the exception it is supposed to raise (and the documentation seems to cover this case), combined with the fact that the documentation directs most applications *not* to call this method, suggests that your change is the better one.

Please address the above comments and re-submit. Thanks again!

comment:3 Changed 8 years ago by Jeremy Thurgood

Author: jerith
Branch: branches/MultiService-remove-named-nonchild-fix-6885

(In [41038]) Branching to 'MultiService-remove-named-nonchild-fix-6885'

comment:4 Changed 8 years ago by Jeremy Thurgood

(In [41040]) Apply MultiService-remove-named-nonchild-fix-6885.diff

refs #6885

comment:5 in reply to:  2 Changed 8 years ago by Jeremy Thurgood

Keywords: review added
Owner: Jeremy Thurgood deleted

Replying to exarkun:

  1. The test methods should be split up so they each exercise only one case or feature. A weak rule of thumb to use is that there should only be one assertXXX call per test method.

The tests I added each only exercise one case (unless I'm missing something) but they have multiple assertions to (a) make sure the initial state is what we expect and (b) assert different parts of initial and final state. I'm happy to change this if you'd prefer it done differently.

  1. The test methods should all have docstrings.

Fixed.

  1. There are some style issues:
    1. Two blank lines between methods
    2. Use more expressive variable names
    3. Please use some more specific language than "more cleanly" in the news fragment

All fixed.

I considered whether changing the behavior of MultiService.removeService to match the interface documentation is better or worse than doing the reverse. Considering IServiceCollection.removeService does actually document the exception it is supposed to raise (and the documentation seems to cover this case), combined with the fact that the documentation directs most applications *not* to call this method, suggests that your change is the better one.

The exception could go either way, but I think removing an entry from MultiService.namedServices before failing is a bug. Not only does it incorrectly change the internal state, it leaves it invalid and causes any future removeService call with the correct named child to fail. All of this is only relevant for code that calls removeService directly (or mucks about with Service.parent), though, since calling it through disownParentService will go via Service.parent anyway.

Ready for another round of review. Thanks!

comment:6 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jeremy Thurgood

The tests I added each only exercise one case (unless I'm missing something) but they have multiple assertions to (a) make sure the initial state is what we expect and (b) assert different parts of initial and final state. I'm happy to change this if you'd prefer it done differently.

For example, test_disowningNamedChild does two things: it calls setServiceParent and then asserts that the child service has become a child of the parent service; then it calls disownServiceParent and asserts that the child service is no longer a child of the parent service. This seems like two tests to me (at least one of which, I hope, is redundant with some existing test since it is somewhat basic functionality).

Separately, even though these three assertions are all part of the same test:

    self.assertEqual(list(parent), [])
    self.assertEqual(child.parent, None)
    self.assertRaises(KeyError, parent.getServiceNamed, "hello")

If one of them fails then any of the following assertions won't even run. The consequence of this is that multiple runs of the test suite are required (with intervening fixes to the implementation) to learn how the implementation is broken. A test suite which reveals all problems in a single run is more useful than this. I might rewrite this as two tests. One which includes this assertion:

    self.assertEqual(
        ([], None),
        (list(parent), child.parent),

and one that includes this assertion:

    self.assertRaises(KeyError, parent.getServiceNamed, "hello")

Or possible as three tests (and avoid the tuple-comparison in the first test by making an assertion about only one of list(parent) and child.parent in each test).

This is all somewhat sad and I think a big step forward in our testing strategy would be to adopt assertThat from testtools. But until that happens I think what I've suggested above is preferable to writing test methods with long sequences of assertions.

Thanks again!

comment:7 in reply to:  6 Changed 8 years ago by Jeremy Thurgood

Keywords: review added
Owner: Jeremy Thurgood deleted

Replying to exarkun:

For example, test_disowningNamedChild does two things: it calls setServiceParent and then asserts that the child service has become a child of the parent service; then it calls disownServiceParent and asserts that the child service is no longer a child of the parent service. This seems like two tests to me (at least one of which, I hope, is redundant with some existing test since it is somewhat basic functionality).

The first part (setServiceParent and the assertions) is setup rather than test code. I could remove the assertions, but then a change in setServiceParent's behaviour can silently break test_disowningNamedChild even if testNamedChild is updated for the new behaviour. I could also avoid calling setServiceParent by manually setting all the expected state, but that's even more fragile to changes in behaviour. I like both of those options even less than the current implementation, but I'm happy to defer to your preference here.

If one of them fails then any of the following assertions won't even run. The consequence of this is that multiple runs of the test suite are required (with intervening fixes to the implementation) to learn how the implementation is broken. A test suite which reveals all problems in a single run is more useful than this. I might rewrite this as two tests.

I could also replace the assertRaises with an equality check on MultiService.namedServices (which is part of the public API anyway) and keep them all in one test with a longer tuple comparison.

comment:8 Changed 7 years ago by Glyph

Keywords: review removed
Owner: set to Jeremy Thurgood

exarkun's previous review points out some aesthetic considerations about the style of the tests which I generally agree with but don't really have a basis in any of our coding standard documentation. Certainly it is very unfortunate that a minor fix like this has sat around for 7 months due to such considerations :-(.

Some further aesthetic considerations which are also apparently not documented in our coding standard:

  1. The style of test docstring that we generally prefer is one that describes the attribute of the system under test that is being verified, rather than describing the verification itself. So for example, instead of
            """
            MultiService.removeService() should raise a ValueError and not modify
            internal state if the named service it's given is not a child.
            """
    

we would write

        """
        MultiService.removeService() raises a ValueError and if the named service it's given is not a child.
        """

In other words, remove words like "should" and "verifies that" (and "internal state" since there are actually no assertions about that - describe the external behavior that you're verifying).

  1. When referencing a named Python code artifact, use epytext markup ("L{}") and not just parentheses at the end of the name, even in tests.
  2. Normally I'd ask for a docstring since you're editing the implementation of a method, but it seems that there's already interface documentation and that the interface documentation already explains the behavior of the method that you're implementing so there's nothing to change :-).

If I might suggest a refactoring of the tests that might make both you and exarkun happy: one of the issues with the multiple assertions is that they are somewhat repetitive. You have sanity-check assertions sprinkled around various tests, many of which are duplicated. Simply make a function that takes a testCase and perhaps a MultiService which performs the set-up and the sanity-check in one step, and then leave the assertions in the test body itself to verify the properties described by the test description. That way, if a test fails a sanity check, at least the traceback points into some clearly-labeled set-up code rather than the bodies of a bunch of random tests. Also, less duplication is always good.

Assuming that buildbot is happy (the python 3 failures are unrelated, a separate regression I need to look into), please address the points above to your satisfaction and then land.

Note: See TracTickets for help on using tickets.