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

Expect tests for generated code in examples/ #230

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

mbarbin
Copy link
Contributor

@mbarbin mbarbin commented Jan 13, 2024

Hello team,

Following our discussion in issue #204 , I've made a small adjustment to the testing workflow. This is a minor change, but I believe it could reveal beneficial.

Here's what's been done:

  1. Expect Tests: For each ml and mli file generated by a dune rule in the src/examples/ directory, corresponding *.ml.expected and *.mli.expected files have been added. This will help us keep track of the impact of PRs on the generated code.

  2. Dune Rule: A dune rule has been added to diff the generated files against their expected reference as part of dune @runtest. In case the generated code changes, updating the expect files is straightforward. You just need to run dune promote, and the expect files will be updated by dune to match the new output.

This change doesn't introduce any new dependencies or affect the non-testing code. It's a small step towards continuing to improve on the existing testing workflow.

I find that it can be hard to know beforehand whether this new test setup will add undue friction. If in practice, this change doesn't work as well as I hope, it can easily be reverted without impacting the rest of the code.

Looking forward to your feedback.

@c-cube c-cube merged commit 1aa73e8 into mransan:master Jan 14, 2024
2 checks passed
@c-cube
Copy link
Collaborator

c-cube commented Jan 14, 2024

Thank you, that makes sense!

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 this pull request may close these issues.

2 participants