TAP 7: Add return code rationale #40
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tap7.md
Outdated
| threshold number of signatures, etc.) in order to simplify work for Updater | ||
| implementers. This will entail more test and control cases, but should make it | ||
| easier for implementers to use the Conformance Tester without having to make | ||
| substantial changes to Updaters or having to too much extra work in writing |
There was a problem hiding this comment.
One too many to too ?
awwad
force-pushed
the
return_code_rationale
branch
from
e5c9921
to
1a0c840
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added a very brief blurb in the TAP on why we're going with two expected return values (from the Wrapper's `updateinstead of many. Here's more of my thinking, for reference. Also caught in this PR are two minor cleaning edits in commit e5c9921.
Argument 1: We don't need specific error codes.
I can't think of a case where we actually need specific error codes to test behavior. We're testing updaters, so we can always ask the question "Did it update without manipulation X and not update with manipulation X?"
Argument 2: Many error codes may be onerous.
To demand that an updater issue distinct errors that percolate up through the code may be an onerous requirement on an implementer, and we want people to not hesitate to use the conformance tester. I'd rather we do extra work in one place to construct good test cases than every implementer work to adapt their updaters to issue error codes that exactly match our expectations.
Argument 2.5: Many error codes encourages test mode divergence from live code.
While test mode could be required for some implementations, the more logic we demand of it, the further it could drift from their live, production code. In the ideal solution, a Wrapper module is thin and it is possible to have conformance tests run with every build of live code, but that can only work if we can test live code or test with only minimal modifications, preferably contained in a static Wrapper module.
Argument 3: Many error codes requires detailed and precise specification of behavior.
The expected behavior if an updater contacts one mirror with invalid metadata and one mirror with valid metadata is simply to update, right? Even the reference implementation throws errors from other mirrors away if one mirror works out. Is that what we expect, or do we expect to get an array of responses per mirror?
A dictionary of responses per mirror means we're specifying that format. It's one more hoop for implementers to jump through.
In the absence of a successful update from one mirror, it might be perfectly sensible for a given Updater not to protest if it finds invalid metadata, but just not accept it and try again a few times or contact the most authoritative source.
I'm sure there would be debates about which error code is more appropriate in a particular scenario.