This is a draft specification for implementing large scale charm tests.

This is a draft specification for implementing large scale charm tests.

https://code.launchpad.net/~clint-fewbar/juju/charm-tests-spec/+merge/91503

(do not edit description out of merge proposal)

[SpamapS, 2012-02-03 20:52:13]

Please take a look.

[niemeyer, 2012-02-06 17:10:57]

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?

[SpamapS, 2012-02-06 21:20:30]

Please take a look.

[SpamapS, 2012-02-06 21:21:30]

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#newcode61
source/charm-tests.rst:61: * The CWD is the charm root directory
On 2012/02/06 17:10:57, niemeyer wrote:
> 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.

My thinking by using the charm root was that bits of the charm may prove useful
in testing. However, in the interest of separation of tests and code, this is
perhaps misguided. Agreed that it should be moved to the tests/ dir to encourage
updating of the tests independent of coding.

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
On 2012/02/06 17:10:57, niemeyer wrote:
> 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. 

Indeed, getting a little ahead of myself there as I am eager to provide a way to
test upgrades, but I think its more than logical to just defer the way to do
this to the next iteration.

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
On 2012/02/06 17:10:57, niemeyer wrote:
> Read-only, maybe?  It needs to be accessible for reading purposes or the
command
> doesn't work.

The wrapper will have access to it, and its even possible it will be there and
readable, however I wanted to assert that tests will not be able to inspect this
directory to pull out any clues about the environment. In part also I don't want
the tests being able to inspect the access key/secret key. Its purely for
encapsulation, and I can remove it, since leaving it out of the guarantees is
effectively the same as denying access.

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

On a failure I want to keep the wget.log around for inspection.

[SpamapS, 2012-02-23 22:47:37]

Please take a look.