apiserver/testing.FakeAuthoriser is not a good mock

[William Reade]

On Thu, Jul 31, 2014 at 7:30 AM, David Cheney <david.cheney at canonical.com>
wrote:
>
> Proposal: remove Authoriser.GetAuthEntity() and replace all calls with
> Authoriser.GetAuthTag(). I have a branch for this, it's < 30 lines as
> there are only 3 places in the apiserver where we do this and they
> usually call the .Tag method on the entity they get back from
> GetAuthEntity.
>

SGTM.

2. This extends from point 1, while the svrRoot derives everything
> from svrRoot.entity, the FakeAuthoriser allows the caller to pass a
> unique value for the tag and the entity of the authorisee. This leads
> to impossible situations where the FakeAuthorizer returns nil for
> AuthGetEntity but a non nil value for AuthGetTag. Other permutations
> exist in our tests and are expected by the test logic.
>
> Propsal: Once step 1 is fixed the difference between svrRoot and
> FakeAuthoriser is the former starts from a state.Entity and derives a
> tag from which other values are derived, the latter skips the initial
> step and starts from a tag. This work falls out of the solution to
> steps 1 and 3.
>
> 3. The helper methods, AuthMachineAgent(), AuthUnitAgent() on the
> Authoriser interface are implemented differently in svrRoot and
> FakeAuthoriser. In tests it is quite common to take the FakeAuthoriser
> from the test suite, copy it and change some of these values leading
> to impossible situations, ie, the tag or entity of the FakeAuthoriser
> pointing to a Unit, but AuthMachineAgent() returning true.
>
> Proposal: The simplest solution is to copy the implementation of these
> helper methods from svrRoot to FakeAuthoriser. A more involved
> solution would be to factor these methods out to be functions in the
> apiserver/common package that take an Authorizer. This second step may
> not pay for itself.
>

I would strongly favour the latter solution. The benefit of having a fake
authorizer whose behaviour diverges is that (at least in theory) it allows
for comprehensive testing against arbitrary authorizer behaviour;
constraining that behaviour so we're only testing realistic situations will
be great, but I fear that doing so by copy-pasting code will only lead to
more divergences in future. Let's make sure we're using the same logic
across the board.

Cheers
William

These steps resolves the majority of the discontinuity between the two
> implementations and will resolve a set of blocking issues I've hit
> converting the apiserver and state packages to speak tags natively.
>
> Thanks for your time
>
> Dave
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20140731/350a78c5/attachment.html>