Update letop-punning tests, add note to CONTRIBUTING #2748
Conversation
CONTRIBUTING.md
Outdated
| The remainder of the tests are so called "golden" or "expect" tests. These tests all consist of running `ocamlformat` on | ||
| `.ml` files and comparing the output to expected output stored in reference files. The `dune` | ||
| configuration for this is automatically generated for you based on the files present in these directories. | ||
| The most tests of `ocamlformat` are so called "golden" or "expect" tests. These |
There was a problem hiding this comment.
I think there is a small typo here
Julow
left a comment
Julow
left a comment
There was a problem hiding this comment.
That's really awesome ! I added some suggestions but your addition is already better than what we had before.
CONTRIBUTING.md
Outdated
|
|
||
| To add a test showing currently incorrect behavior, add a `.ml` file to | ||
| `test/failing/tests`. If command line arguments are needed, create a | ||
| corresponding `.opts` file with the same base name. The output of these tests |
There was a problem hiding this comment.
Base name is ambiguous here, I think you should use entire file names. For example, it's not mytest.opts but mytest.ml.opts
| output to expected output stored in reference files. The `dune` configuration | ||
| for this is automatically generated for you based on the files present in these | ||
| directories. | ||
|
|
There was a problem hiding this comment.
I think it would be useful to mention running dune runtest here. The linked "Running the Tests" section talks about several things but tests added following this guide is specifically run with dune runtest.
EmileTrotignon
added
the
no changelog
EmileTrotignon
left a comment
EmileTrotignon
left a comment
There was a problem hiding this comment.
The CI failures are weird and look unrelated to this PR. I think this is ready to be merged.
|
The test failures on those platforms have been coming and going on different commits, most of which are only updating the markdown, so I agree. Let me know if there is anything else needed to merge this! |
3 participants
Requested by @Julow here.
This fixes up the letop-punning tests to avoid duplicating the same .ml file (and adds a
test/failingcase that I will resolve in a separate PR), and adds a section to CONTRIBUTING.md that should help others in the future get onboarded with the testing system.