mozilla-services/android-sync — comment on commit 563dc5294f7a3e2734d789b5880b0ecc063deedd
File:  line 0
By ncalexan:

I will respond only to a subset of your points, and then suggest how I think we should move forward.

> In this commit you're reducing test coverage; you should always try to explain why that is.

I deleted the tests individually so that I could highlight what was in place for each deleted test for you as reviewer (rather than writing a long commit comment or even worse a GitHub comment.) In the case I found where the SyncCryptographer test was not a dupe, I adapted it.

In future I will be more conscientious about identifying grouped commits whose effects must be considered as a whole.

> Note that testSetKeysFromWBO — as I mentioned in my comment — does not parse a real server-side record; it constructs one in a different way from a JSON *payload*, not a complete envelope. There absolutely should be a test that covers the whole process, and you're deleting the last such test! That we don't use SyncCryptographer doesn't mean that testing it is pointless: *it* uses other code that really is live.

There should be a test that covers the whole process *as it is implemented in the code*, but it is my opinion that the SyncCryptographer tests were actively bad: SyncCryptographer implements something that you *believe* is the same as the whole process and you *believe* it is well tested. But SyncCryptographer may or may not implement the same process. (I claim that the fact that it uses code that is live is beside the point as well: I have faith in our ability to encrypt/decrypt because we carefully test the live building blocks, not because the mangled carcass of SyncCryptographer happens to exercise a code path that happens to use such building blocks.)

Now, for moving forward: how about I commit a patch that adapts these deleted tests and we leave arguing about whether things are minimal/duped to the dogs?