-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve clarify of errors, fix broken tests #69
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
This is a rather large PR that got out of hand 😅 But it fixes and improves quite a few things. There is a lot of parsing going on left and right by various parts: reading the files, the external test tools, and parsing of what they output. Previously it was *really* hard to grok what was parsing what and what exactly failed. It took me ages to grok why these tests were failing for the TOML library; turns out this was a mixture of actual bugs in the TOML library, bugs in the toml-test-* tools, bugs in this test tool, and tests people had added that were just broken. This vastly redoes the test output to look quite nice and understandable (see below), it's also a lot more verbose, but that's a feature IMO. Instead of just "expected type string, got int" actually show what we have. There is some room for improvement here as it shows this with fmt.Printf("%#v"), which gives rather Go-specific output. Maybe show as JSON? Have to see later. Some other changes as well: - All JSON files are formatted consistently according to what json.MarshalIndent(.., "", " ") outputs. Being predictable means it's much easier to compare the desired and actual outputs since you don't have to mentally parse the differences in formatting so much. - Fix a bunch of broken tests, and arrays can now be represented as just "[ ... ]"; no object with type/value needed (or even supported). It was added in case "TOML might support tuples", but we'll cross that bridge if we ever get at it. Fixes #52 Fixes #58 Fixes #67 - Tests are now consistently referenced with the full path relative to the test directory: "valid/name" and "invalid/name". e.g. to run one test use "toml-test [..] valid/name" This means it's clearer what kind of test we're dealing with, means we don't need globally unique tests, and that we can add "js-valid" tests specifically for a language if need be. All filesystem access now also done trough the fs.FS interface; the big upshot of this is that we can produce binaries with the tests embedded in them and that you don't need to pass -testdir (this still works, for development etc.) The downside is that it requires Go 1.16, but I think that's okay for a tool like this. We can just provide binaries once a new release is tagged. Fixes #20 Fixes #23 ---- Example output (test name and some other parts are in bold unless you add -no-bold): Test: invalid/key-multiline (toml-test-decoder < ../toml-test/tests/invalid/key-multiline.toml) Expected an error, but no error was reported. input sent to toml-test-decoder: """long key""" = 1 output from toml-test-decoder (stdout): { "long\nkey": { "type": "integer", "value": "1" } } want: <empty> Test: valid/inline-table-nest (toml-test-encoder < ../toml-test/tests/valid/inline-table-nest.json) Error encoding TOML: toml: TOML array element cannot contain a table input sent to toml-test-encoder: { "arr_arr_tbl_empty": [ [ {} ] ], [.. trim ..] output from toml-test-encoder (stderr): Error encoding TOML: toml: TOML array element cannot contain a table want: <empty>
arp242
added a commit
to BurntSushi/toml
that referenced
this pull request
Jun 14, 2021
Update to use the new toml-test from toml-lang/toml-test#69 Turns out quite a few failures were just because those tests were broken. Do'h. And fix a few minor issues: - nan and inf got encoded as "NaN" and "Inf". - Detect invalid UTF-8 and error out. - Don't allow keys as triple-quoted strings: """ key """ = "foo" - Allow encoding blank keys ("" = 1) and escaped in keys ("\n" = 1). - Expand string quoting with \b and \f, and other control characters as \u...
arp242
added a commit
to BurntSushi/toml
that referenced
this pull request
Jun 14, 2021
Update to use the new toml-test from toml-lang/toml-test#69 Turns out quite a few failures were just because those tests were broken. Do'h. And fix a few minor issues: - nan and inf got encoded as "NaN" and "Inf". - Detect invalid UTF-8 and error out. - Don't allow keys as triple-quoted strings: """ key """ = "foo" - Allow encoding blank keys ("" = 1) and escapes in keys ("\n" = 1). - Expand string quoting with \b and \f, and other control characters as \u... - Allow multiline strings to end with the quote character: """v"""" The toml-test tests will fail because of a catch-22: the toml-test needs this version of the toml library to work for the fix in the lexer for the multiline strings.
This is amazing! Lovely work. Improving failure modes is the secret to success! |
Thanks! |
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.
This is a rather large PR that got out of hand 😅 I wanted to get the
failing tests for the toml library passing, and one thing led to
another.
I suppose this could be split out in multiple PRs, but is anyone going
to review this? Probably not.
Either way, it fixes and improves quite a few things.
There is a lot of parsing going on left and right by various parts:
reading the files, the external test tools, and parsing of what they
output. Previously it was really hard to grok what was parsing what
and what exactly failed.
It took me ages to grok why these tests were failing for the TOML
library; turns out this was a mixture of actual bugs in the TOML
library, bugs in the toml-test-* tools, bugs in this test tool, and
tests people had added that were just broken.
This redoes the test output to look quite nice and understandable
(see below), it's also a lot more verbose, but that's a feature IMO.
Instead of just "expected type string, got int" actually show what we
have. There is some room for improvement here as it shows this with
fmt.Printf("%#v"), which gives rather Go-specific output. Maybe show as
JSON? Have to see later.
Some other changes as well:
All JSON files are formatted consistently according to what
json.MarshalIndent(.., "", " ") outputs. Being predictable means it's
much easier to compare the desired and actual outputs since you don't
have to mentally parse the differences in formatting so much.
This is also mostly similar to what e.g.
jq
outputs.Fix a bunch of broken tests, and arrays can now be represented as just
"[ ... ]"; no object with type/value needed (or even supported). It
was added in case "TOML might support tuples", but we'll cross that
bridge if we ever get at it. A bunch of tests were broken because they
didn't have this, and I had to either add or remove it, and removing
it seemed to make the most sense.
Fixes Inconsistent handling of array of tables #52
Fixes datetimes differ in JSON and TOML #58
Fixes Consistent floating point representation between tests #67
Tests are now consistently referenced with the full path relative to
the test directory: "valid/name" and "invalid/name". e.g. to run one
test use "toml-test [..] valid/name"
This means it's clearer what kind of test we're dealing with, means we
don't need globally unique tests, and that we can add "js-valid" tests
specifically for a language if need be.
All filesystem access now also done trough the fs.FS interface; the
big upshot of this is that we can produce binaries with the tests
embedded in them and that you don't need to pass -testdir (this still
works, for development etc.)
The downside is that it requires Go 1.16, but I think that's okay for
a tool like this. We can just provide binaries once a new release is
tagged.
Fixes When valid and invalid tests have the same name, can't specify which to run #20
Fixes language specific tests? #23
Example output (test name and some other parts are in bold unless you
add -no-bold):