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

Generate tests from the spec? #129

Closed
epage opened this issue Jan 16, 2023 · 8 comments · Fixed by #131
Closed

Generate tests from the spec? #129

epage opened this issue Jan 16, 2023 · 8 comments · Fixed by #131

Comments

@epage
Copy link
Contributor

epage commented Jan 16, 2023

As far as I can tell, I do not see a test for this rule within the spec

The [table] form can, however, be used to define sub-tables within tables defined via dotted keys.

I was starting to write a test but I realized we could possibly code-generate tests from the spec seeing as the spec follows a specific pattern in marking whole documents invalid or specific parts.

Before starting on that, I wanted to gauge interest and preferred method for pulling in the toml.io repo (sub-module, sub-tree, having users do it by hand, etc) and if there is anything with versioning we should do (I feel like the json should be extended with versioning information rather than embedding it within the binary but the simplest to unblock is to only generate for the latest spec).

@arp242
Copy link
Collaborator

arp242 commented Jan 16, 2023

As far as I can tell, I do not see a test for this rule within the spec

The [table] form can, however, be used to define sub-tables within tables defined via dotted keys.

I thought there's a test for this, but maybe not as I can't find it now 🤔

Closest are https://github.com/BurntSushi/toml-test/blob/master/tests/valid/table/array-nest.toml and https://github.com/BurntSushi/toml-test/blob/master/tests/valid/table/array-table-array.toml

Maybe I'm missing something; need to verify later.

I realized we could possibly code-generate tests from the spec seeing as the spec follows a specific pattern in marking whole documents invalid or specific parts.

How exactly would you do this? As in, I can see how you can take the specification and generate a lot of permutations based on that, but I'm not sure if that's really a good replacement for the tests we have now. One of the nice properties we have now is that tests are "grouped" and you can easily ignore a set of them, which is very useful especially when developing new implementations.

We could perhaps add them as their own set of tests though.

I feel like the json should be extended with versioning information rather than embedding it within the binary but the simplest to unblock is to only generate for the latest spec

We don't have a JSON file for the "invalid" tests; that's why it's in the binary.

Maybe it should be encoded in the filename, or make it more obvious by grouping things in directories. I know quite a few people don't use the toml-test program/protocol but just iterate over the test files. Not 100% sure what the best approach is for this.

@epage
Copy link
Contributor Author

epage commented Jan 16, 2023

I thought there's a test for this, but maybe not as I can't find it now 🤔

The closest I found were:

How exactly would you do this? As in, I can see how you can take the specification and generate a lot of permutations based on that, but I'm not sure if that's really a good replacement for the tests we have now. One of the nice properties we have now is that tests are "grouped" and you can easily ignore a set of them, which is very useful especially when developing new implementations.

My thought that these auto-generated cases wouldn't be meant to replace existing tests and would live in a valid/spec and invalid/spec directories. The spec examples are fairly minimal and don't cover a lot of the corner cases as the main tests, so if someone really can't cover them, they can skip the entire spec directory (or specific tests within it). If they can cover them but can't handle every use case, they the regular grouping works as needed.

For more background: I was passively considering this idea but then I realized I was going to write a test that basically copies the spec, so I should ask about that idea first.

I know quite a few people don't use the toml-test program/protocol but just iterate over the test files

The two main Rust parsers do this (I'm the maintainer). For the one test for the unreleased version, I just have it skipped for now.

@arp242
Copy link
Collaborator

arp242 commented Jan 16, 2023

My thought that these auto-generated cases wouldn't be meant to replace existing tests and would live in a valid/spec and invalid/spec directories. The spec examples are fairly minimal and don't cover a lot of the corner cases as the main tests, so if someone really can't cover them, they can skip the entire spec directory (or specific tests within it). If they can cover them but can't handle every use case, they the regular grouping works as needed.

For more background: I was passively considering this idea but then I realized I was going to write a test that basically copies the spec, so I should ask about that idea first.

I think adding this would make sense.

The two main Rust parsers do this (I'm the maintainer). For the one test for the unreleased version, I just have it skipped for now.

Speaking of which, I was updating the TOML wiki earlier today and it wasn't clear which TOML version https://github.com/toml-rs/toml supports; it's not mentioned in the README or in any of the source files (as far as I could find). I just assumed it supports 1.0.0 as it seemed maintained, but might be a good idea to mention this in the README or somewhere.

@arp242
Copy link
Collaborator

arp242 commented Jan 16, 2023

https://github.com/BurntSushi/toml-test/blob/master/tests/valid/inline-table/key-dotted.toml

Ah right; this one basically tests the behaviour with [tbl] and [tbl.x].

A lot of these tests could be grouped better.

@arp242
Copy link
Collaborator

arp242 commented Jan 16, 2023

Added a new test in 102e04e

@epage
Copy link
Contributor Author

epage commented Jan 16, 2023

I think adding this would make sense.

How would you like the tom.io repo pulled in for extracting the test cases? And should I only generate cases for the latest spec version since the versioning story is still a little weak?

Added a new test in 102e04e

This covers dotted keys in table headers which has a different set of considerations for an implementer than dotted keys in key-value pairs considering some situations can be extended while others can't.

@epage
Copy link
Contributor Author

epage commented Jan 16, 2023

Speaking of which, I was updating the TOML wiki earlier today and it wasn't clear which TOML version https://github.com/toml-rs/toml supports; it's not mentioned in the README or in any of the source files (as far as I could find). I just assumed it supports 1.0.0 as it seemed maintained, but might be a good idea to mention this in the README or somewhere.

I actually do not know the answer to that atm. tomls parser has not been touched much in a while. The situation will soon be changing as I'm working on consolidating the parers (toml-rs/toml#340).

@arp242
Copy link
Collaborator

arp242 commented Jan 16, 2023

How would you like the toml.io repo pulled in for extracting the test cases? And should I only generate cases for the latest spec version since the versioning story is still a little weak?

I'd focus on TOML 1.0.0 to start with, but we do need to add TOML 1.1.0 as well at some point. Maybe the easiest is to just cp the ABNF file to abnf/toml-1.0.0.abnf, abnf/toml-1.1.0.abnf etc. rather than using git submodules or some such. It's unsophisticated, but also straightforward and "just works", and not like these files need updating that often.

But just do whatever you think makes sense. I don't have super-strong opinions.

This covers dotted keys in table headers which has a different set of considerations for an implementer than dotted keys in key-value pairs considering some situations can be extended while others can't.

These are already tested in the valid/key/dotted.toml test, right?

Either way, always happy to add more tests if something is missing.

epage added a commit to epage/toml-test that referenced this issue Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants