Skip to content
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
merged 2 commits into from
Jun 14, 2021
Merged

Improve clarify of errors, fix broken tests #69

merged 2 commits into from
Jun 14, 2021

Conversation

arp242
Copy link
Collaborator

@arp242 arp242 commented Jun 14, 2021

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):

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 2 commits June 15, 2021 02:02
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 arp242 merged commit b201e81 into master Jun 14, 2021
@arp242 arp242 deleted the errs branch June 14, 2021 20:07
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.
@BurntSushi
Copy link
Member

This is amazing! Lovely work. Improving failure modes is the secret to success!

@arp242
Copy link
Collaborator Author

arp242 commented Jun 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants