New testcase system for exercising typed nodes; Revamp struct tests to use it. #66
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
mvdan
reviewed
Aug 24, 2020
rvagg
reviewed
Aug 25, 2020
warpfork
force-pushed
the
schema-testcases-reno
branch
3 times, most recently
from
e2f5e9f to
5910278
Compare
…ith it. This new system focuses on table-driven tests, and leans heavily upon json as a shorthand for expressing fixtures. It also makes a great deal more effort to exercise the different features of nodes (and their paired representation nodes) from all directions at once for each test datum, rather than requring that all be written out manually. The result is that the struct tests we've renovated have a lovely diffstat shrinkage: 111 insertions, 299 deletions... And yet the smaller line count results in *more* coverage. (Okay, the linecount increase for the testcase structure and helper methods is much bigger than the savings in fixture size... but, only *so far*. I assume this will continue to pay off in the future.) Relatedly: a bug in struct map representations has been fixed. (It was the sibling of 5f58965, embarassingly.) Thank goodness we now get proper coverage of this area. There's a few TODOs left to further expand the exercises, but those can slot in easily in subsequent commits. Same goes for further expansion of usage of this new system.
Though in general, letting golang's test system replace spaces and other unusual characters with underscores has very little downside, here, the resulting "_--_" is quite unpleasant on the eyes. Change things to use TitleCase.
warpfork
force-pushed
the
schema-testcases-reno
branch
from
54ac9f6 to
5d732d4
Compare
|
There's still an open TODO in here about adding a Copy feature and using it to make sure both the AssembleKey+AssembleValue path is exercised on map-like stuff in addition to the 2-for-1 AssembleEntry path (which is what's exercised by unmarshalling, and thus currently exercised by this system). But I think I'm going to be okay with that for today and merge this; I want to start writing more tests of other stuff with this system, and not pollute this PR's diff view with that stuff. Rebased and mergin'. |
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.
Introduce some new test helpers, focusing on table-driven tests, and leaning heavily upon json as a shorthand for expressing fixtures.
It also makes a great deal more effort to exercise the different features of nodes (and their paired representation nodes) from all directions at once for each test datum, rather than requiring that all be written out manually, which is how I'd been slogging along previously.
The result is that the struct tests we've renovated have a lovelydiffstat shrinkage: 111 insertions, 299 deletions... but the smaller line count results in more coverage. Yay!
(Okay, the linecount increase for the testcase structure and helper methods is much bigger than the savings in fixture size... but, only so far. I assume this will continue to pay off in the future.)
Relatedly: a bug in struct map representations has been fixed. (It was the sibling of 5f58965, embarrassingly.) Thank goodness we now get proper coverage of this area.
There's a few TODOs left to further expand the some of the tests performed; not sure if I'll append those to this PR before merging or do them in separate small future PRs. Same goes for further expansion of usage of this new system.
There's also some future work to do around the lookup error assertions -- the main sore spot there is that the traversal package doesn't emit very well-typed errors. That's likely best solved in a separate PR since it will mostly involve that package.