Code review comment for lp:~clint-fewbar/pyjuju/charm-tests-spec

Thanks Clint. Here are some comments:

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst
File source/charm-tests.rst (right):

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode51
source/charm-tests.rst:51: utilized to attach tests to charms. Under the
charm root directory,
"To facilitate tests attached to charms .. to attach tests to charms."

The sentence is a bit self-referential. Would be nice to reword

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode60
source/charm-tests.rst:60: * the default environment is bootstrapped
To be more clear, we might say something like:

* A testing environment is already bootstrapped and is made the default
for command line usage.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode61
source/charm-tests.rst:61: * The CWD is the charm root directory
I think this should be the "tests/" directory itself. Note that the test
doesn't actually have a strong relation to the content of the charm. The
content is deployed into the remote machine, and the test is sitting
locally. Its interaction will be mainly remote.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode64
source/charm-tests.rst:64: charm url and/or repository arguments of the
test runner's choice. This
We have to qualify a bit better what the "runner's choice" mean, since a
charmer reading that will feel like "Okay, but _what is the version_?".

Maybe something like "the current version at the moment the test is
run"? This gives us some freedom to move back and forth, and gives the
charmer a vague feeling of what to expect.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode69
source/charm-tests.rst:69: * a special sub-command of juju,
``deploy-previous``, will deploy the
This command doesn't exist. Would you mind to drop the upgrade aspect
from the specification entirely? I'd be happy to debate about a follow
up improvement once we have the basics in place.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode76
source/charm-tests.rst:76: * ``~/.juju`` will not be accessible to the
tests
Read-only, maybe? It needs to be accessible for reading purposes or the
command doesn't work.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode82
source/charm-tests.rst:82: If present, tests/requirements.yaml will be
read to determine packages
Looks good, but can we please name that as "tests/tests.yaml", so that
this may be used for other needs too, when/if they show up?

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode90
source/charm-tests.rst:90: If a tool is needed to perform a test and not
available in the Ubuntu
s/and not/and is not/?

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode111
source/charm-tests.rst:111: running charms, and so may affect the
outcome.
Section looks great.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode141
source/charm-tests.rst:141: trap teardown EXIT
Destroying the temporary state manually should not be necessary. The
harness should be able to put the environment back onto a clean state
without any actions from the test itself. We need to do that anyway, so
we may as well spare people form even thinking about it in their tests.

https://codereview.appspot.com/5624044/diff/1/source/charm-tests.rst#newcode175
source/charm-tests.rst:175: touch $datadir/passed
I think that shouldn't be needed as well, in principle, since we should
always run with -e. What's your thinking around this?

https://codereview.appspot.com/5624044/