[MERGE] Deprecate old fns that use InventoryEntry methods that ought to go

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> John Arbash Meinel wrote:
>> Oh, we should make sure there is a test that it is deprecated... It
>> seems unfortunate that we have no test code exercising this already that
>> you can just switch to 'self.callDeprecated(change_entry)'
> 
> John,
> 
> Are you saying that every time we ever deprecate something, we should
> have a test that it is deprecated? I agree that's appropriate in certain
> cases but, as a blanket rule, it sounds a little excessive to me. I
> don't think that's really adding value, is it? Once I delete the code, I
> don't expect a test confirming that it no longer exists to stretch the
> analogy further.
> 
> FWIW, we have 70+ deprecated symbols in the code base and very few tests
> (bar the generic ones) along these lines. I'm no TDD expert. I'd add it
> if you or anyone else feels strongly about it being the right thing to
> do. But it feels like I'm being asked to introduce a call to be piece of
> code when there are zero other known calls to that code in the universe
> and I want the code nuked at the earliest legal point in time I can.
> 
> My 2c,
> Ian C.
> 
> 

In general, all functions should already be tested (obviously this is not 100%
true).

When we deprecate a function, we are actually saying "you shouldn't be using
this function, but we still guarantee that it works.".

Which means that we can't remove the tests for functions we deprecate, because
we still are guaranteeing to the users that it works the way it used to. We are
just recommending that they update to be used a different way.

Once we finally *remove* a function, then we can get rid of the tests for it.

Otherwise you run into deprecating a function, and then accidentally breaking
it *during* the deprecation period. Where it is still "officially supported".

For *this* particular function, we didn't have any tests. So I don't even know
what "correct" behavior is. And I don't expect you to work that out, and write
tests for a function we plan on removing anyway. It isn't worth your time. It
*might* be worthwhile to add a single test that makes sure calling this
function generates the deprecation warning. Or some basic smoketest that the
function still does something.

If we are at the point that we nuke the code, and we have a test which is
failing because self.callDeprecated() is failing, it is obvious that we can
nuke that test at the same time.



So ultimately, in this situation, I think what you did is fine. In other
instances where we actually do have a tested function going through
deprecation, we should not delete their tests, but update them to handle the
deprecation warnings.

And you don't need to deprecate the private function.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGn4hxJdeBCYSNAAMRAjveAJwNdkail/Geuwophg2a5Let6rf+qwCgwzvX
rBSt3qQScoIn3HtW2w5lDqM=
=RTKt
-----END PGP SIGNATURE-----