Code review comment for lp:~niemeyer/juju-core/mstate-machine-watcher

Revision history for this message
Aram Hăvărneanu (aramh) wrote :

> The reasoning for doing it doesn't sound great. Doing DeepEquals on
such
> a structure is most definitely wrong. Private data is private, and by
> definition we should be able to cache information freely or change it
> without breaking tests that work on the public APIs.

It may be wrong, but in that case we have to do lots of work to fix it
because all the documents will have this field.

  white:mstate$ cd state
  /home/aram/src/launchpad.net/juju-core/state
  white:state$ g DeepEquals | wc -l
  89
  white:state$ cd mstate
  /home/aram/src/launchpad.net/juju-core/mstate
  white:mstate$ g DeepEquals | wc -l
  58

To be honest, using DeepEquals in tests always felt wrong to be because
it was checking the implementation details instead of relying on the
interface, but I assumed it was ok since this is the way state tests
were written in the first place.

I will put omitempty, but this doesn't fix the tests. Entities returned
by Insert will have TxtRevno always zero but entities returned by other
querying functions or by Refresh will have a non-zero TxtRevno.

If you are against this change, perhaps an alternative is to call
Refresh() in tests where this might matter.

https://codereview.appspot.com/6489104/

« Back to merge proposal