Code review comment for lp:~thumper/juju-core/testing-docs

Revision history for this message
William Reade (fwereade) wrote :

This is awesome. Not all my comments are necessarily directly
actionable, in that they're complaints about our current testing, but
perhaps you can find some way to work their intent in usefully.

https://codereview.appspot.com/96110043/diff/1/doc/how-to-write-tests.txt
File doc/how-to-write-tests.txt (right):

https://codereview.appspot.com/96110043/diff/1/doc/how-to-write-tests.txt#newcode119
doc/how-to-write-tests.txt:119: // a pointer to that variable so we can
patch it in the tests.
formatting

https://codereview.appspot.com/96110043/diff/1/doc/how-to-write-tests.txt#newcode151
doc/how-to-write-tests.txt:151: github.com/juju/testing, which brings in
the CleanupSute from the same.
s/Sute/Suite/

https://codereview.appspot.com/96110043/diff/1/doc/how-to-write-tests.txt#newcode159
doc/how-to-write-tests.txt:159: JUJU_ENV, and JUJU_LOGGING_CONFIG
environment variables.
The FakeHomeSuite should generally not be used, because it assumes a
context of a running (cmd/juju) client; almost all the code should avoid
making this assumption, because it's actually generally going to run on
an API server.

https://codereview.appspot.com/96110043/diff/1/doc/how-to-write-tests.txt#newcode162
doc/how-to-write-tests.txt:162: api server.
The JujuConnSuite is all fucked up, and should be used when you just
don't care about having coherent context available, you just kinda want
everything and don't want to think too hard. State server? Client? Both?
Whooo! Who cares!

Yeah, we should fix this; but in the meantime I want people to know it's
at least somewhat bad/wrong, because then *maybe* someone will take it
upon themselves to extract a saner subset of the functionality and start
using that; as it is it's all too easy to just allow the rot to gently
spread without even knowing that's what you're doing.

https://codereview.appspot.com/96110043/diff/1/doc/how-to-write-tests.txt#newcode215
doc/how-to-write-tests.txt:215: // of the test.
Really, FWIW, I think we should always be clearing the env at the very
start of a test.

https://codereview.appspot.com/96110043/

« Back to merge proposal