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

Contradicting tests invalid/no-seconds.toml and valid/no-seconds.toml #134

Closed
erbsland-dev opened this issue May 10, 2023 · 6 comments
Closed

Comments

@erbsland-dev
Copy link

In the HEAD version from the repository (commit 01cfece), There are tests in the invalid and valid directory, both testing for date/time values without seconds.

  • invalid/no-seconds.toml
  • valid/no-seconds.toml
@eksortso
Copy link

Times without seconds were invalid in TOML v1.0.0. But they are now valid, as of PR 894 of toml-lang/toml, so they will be valid when the first RC of TOML v1.1.0 is released. I don't know what the plans will be for the invalid tests in the future, but one way or another, they'll be gone.

@arp242
Copy link
Collaborator

arp242 commented May 10, 2023

Yeah, what @eksortso said.

Some tests only apply to specific versions; there's a list in https://github.com/BurntSushi/toml-test/blob/master/version.go

By default only the 1.0 tests are run with toml-test; you need to use toml-test -toml next to select the newer version. If you don't use the toml-test binary you'll need to duplicate that.

Maybe the filenames should be updated to make this more obvious as this confusion has come up before and it will only become more confusing as TOML 1.1, 1.2, etc. get released in the future.

I don't know what the plans will be for the invalid tests in the future, but one way or another, they'll be gone.

The general idea is to remain compatible with all TOML versions whenever possible, since many libraries tend to be fairly slow in updating. In hindsight I should have done this when I updated this from 0.4 to 1.0 since it caused me some pain. Live and learn.

@erbsland-dev
Copy link
Author

Adding the version in the filenames would be great. I am integrating the test files directly into the unit test for the parser (will be published soon), as I try to have as little dependencies possible.

@arp242
Copy link
Collaborator

arp242 commented May 10, 2023

The issue is that I would prefer the filenames to remain stable, so people can skip them and safely update toml-test without worrying about which tests moved around. In the own TOML parser I maintain I skip some minor things because they're fairly hard to fix and, IMHO, not worth the bother.

On the other hand, by adding new TOML 1.1 tests also means people can't safely update, since TOML 1.0 parsers will break.

I'm not 100% sure yet how to best handle this; I will reopen this issue as a reminder that something needs to be done.

@arp242 arp242 reopened this May 10, 2023
@erbsland-dev
Copy link
Author

What about adding a version.json file that contains the arrays with included/excluded files?
It would allow to update the test files without altering the test application.

@arp242 arp242 closed this as completed in 9a4eff2 May 17, 2023
@arp242
Copy link
Collaborator

arp242 commented May 17, 2023

Yeah, that's probably the best solution.

I added a new -list-files flag which will list all the test files for the given -toml flag; it's just one-file-per-line rather than JSON, which is a bit easier to use if you want to copy all the files from the CLI and such.

To make things easier for other people I also added a pre-generated tests/files-toml-1.0.0 and tests/files-toml-1.1.0 which contains the output of that so people don't need to run toml-test themselves.

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

No branches or pull requests

3 participants