environs/*: simplify config tests

environs/*: simplify config tests

This is an alternative to factoring out the
common code (as in https://codereview.appspot.com/7225082).
Most of the code was unnecessary, so the factoring
is perhaps no longer so compelling.

https://code.launchpad.net/~rogpeppe/juju-core/208-simplify-config-tests/+merge/146190

(do not edit description out of merge proposal)

[rog, 2013-02-01 17:52:37]

Please take a look.

[dimitern, 2013-02-01 17:55:16]

Even better! LGTM

[jameinel, 2013-02-02 16:32:59]

I wonder if we are removing test coverage accidentally. If it is already covered
elsewhere, then LGTM.

https://codereview.appspot.com/7231080/diff/2001/environs/ec2/config_test.go
File environs/ec2/config_test.go (left):

https://codereview.appspot.com/7231080/diff/2001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:71: c.Check(err, IsNil)
You're removing an assertion that you can round trip the config to disk and the
back again.
I know we tripped these tests in the past when we were trying to play around
with config (such that it was decided to not do what we were doing).

Is there a reason to remove them here? (They might also be elsewhere, etc)

[rog, 2013-02-04 14:02:43]

Please take a look.

https://codereview.appspot.com/7231080/diff/2001/environs/ec2/config_test.go
File environs/ec2/config_test.go (left):

https://codereview.appspot.com/7231080/diff/2001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:71: c.Check(err, IsNil)
On 2013/02/02 16:32:59, jameinel wrote:
> You're removing an assertion that you can round trip the config to disk and
the
> back again.
> I know we tripped these tests in the past when we were trying to play around
> with config (such that it was decided to not do what we were doing).
> 
> Is there a reason to remove them here? (They might also be elsewhere, etc)

I think a round tripping test is a good idea, but that's not really
what the original code here was doing - it would have to marshal the
attributes retrieved from the environ to do so.

The original code is just checking yaml marshalling and unmarshalling and the
ReadEnvironsBytes code, both of which are tested adequately elsewhere, I think.
(there's nothing in this package that gets touched until ReadEnvironsFromBytes
calls environProvider.New with the config that's been unmarshalled from the
yaml).

I've added an explicit round trip test.

[fwereade, 2013-02-04 15:11:44]

LGTM

[fwereade, 2013-02-04 16:01:25]

On 2013/02/04 15:11:44, fwereade wrote:
> LGTM

(but please sync up with jtv and find agreement there)

[jtv.canonical, 2013-02-05 02:53:10]

Two points:

1. Don't forget to update environs/openstack/config_test.go as well.

2. I think this is an improvement, and I approve of it.  But I don't think the
tests are set up well in the first place.  For me it would make more sense to
make the roundtrip a separate test, with its own clear meaning.

Arbitrarily tacking extra functionality onto a test on the one hand, and
extending the testing data on the other is an antipattern that has cost us
dearly in Launchpad history.  You get poorly-understood, brittle tests.  It
leads to pathologies such as "change the code, run the tests, fix the test
breakage as simply as possible, add tests for the new code, land."  True,
reviewers shouldn't accept that.  But they will if it's the standard that the
code sets and they find it's the only way to get things done.

So I would favour moving the roundtrip check out of the loop altogether, and
giving it its own test functions.

[dave_cheney.net, 2013-03-13 01:09:57]

LGTM. Thank you.