Fwd: Re: PatchEnvironment and PatchValue on cleanup suite (issue 13770045)

[Tim Penhey]

Moving this to the mailing list, because comments on landed code means
most people don't see it.

> On 2013/09/19 15:15:12, rog wrote:> I just noticed this.
> 
>> "The Set function was moved from testing/checkers to
>> testing.  It wasn't a checker and it has also been
>> renamed to PatchValue, and in testing/patch.go. The
>> PatchEnvironment function has been added to that
>> file too."
> 
>> There was a particular reason that I defined Set inside
>> testing/checkers rather than in testing - dependencies.
>> There are various places that can't import testing because
>> it has a particularly hefty set of dependencies.

Perhaps a better thing is to make a new base testing package.

Almost all of our tests should be based on the testing.LoggingSuite (or
where ever we move it to, in order for all logging to end up in the
gocheck Log for the test.

LoggingSuite now embeds a CleanupSuite that provides us with the
AddCleanup and AddSuiteCleanup methods.

Perhaps we add a new package for these, and the patch methods in order
to limit the dependencies, and to also add a test for that package to
make sure that extra dependencies don't creep in.

>> It may be that "checkers" is not a great name for a package
>> that includes both checkers and the patch functionality,
>> but I think there should be a place for our minimal-dependency
>> testing-related code, and I think that the package we import
>> as "jc" (whatever its path is) is a nice place for that.

The checkers package was created to have exactly just that, checkers.
I'd prefer to keep the clean lines there.

> IMHO, if this were in "checkers", then "checkers" needs to change name.
> Perhaps a new subpackage is in order for small test utilities. Like,
> "testutils" or something. This new subpackage could be required to have
> no dependencies other than third-party (gocheck/loggo),
> testing/checkers, and stdlib.
> 
>> I quite liked the original "jc.Set" name too - easy to type
>> and nicely indicative of its purpose.

The problem I have with this name is that you need to know what it does.
The name doesn't help you.  This adds extra cognitive mapping required.

I come from a background where things were "Monkey Patched".  I much
prefer the PatchEnvironment and PatchValue names as they are more
descriptive as to what they do.

>> Would you object too horribly if I reverted that part of this
>> CL, and moved testing.PatchEnvironment to jc.Setenv ?

Yes actually, I would object horribly.


Tim