Code review comment for lp:~benji/landscape-client/bug-1548946-xenial-test-failures

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

> > > I'm basically +1 with a non-blocking nitpick.
> > >
> > > Just marking as Needs information because of the following question.
> > >
> > > Since it seems you investigated the issue in depth and possible looked at
> > the
> > > changed behavior in the apt module, do you think it would be possible to
> > avoid
> > > using the TestCache and change the test to adapt to the new behavior
> > somehow?
> > > Or is it now plain impossible for calling code to have visibility on that?
> >
> > I spent a (timeboxed) bit of time trying to discern exactly how the cache's
> > behaviour has changed, and I couldn't figure enough out to exploit it for
> our
> > tests.
> >
> > Arguably, exploiting said details is what got us into this situation, but I
> > wouldn't argue that too strenuously.
>
> Right, has all integration-level tests they have a cost and they get to
> failure situations when the other party changes, but flip side is that they
> increase confidence in the behavior of the external system (the apt python
> library in this case) and the way your code in interacts with it.
>
> I'm approving this branch, but I'd suggest to file a bug about this and put an
> # XXX comment pointing to the bug inside the class docstring of the newly
> introduced TestCache.
>
> +1

Ideally the class docstring of TestCache should also explain the background of why it was introduced, summarizing the content of the bug its pointing to (in case one wants to get the full context).

« Back to merge proposal