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

Make -unused-code-warning tests actual tests #492

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

NathanReb
Copy link
Collaborator

In their previous form they were not executed as part of the default build or as part of the runtest alias.

They relied on a deriving_inline workflow which wasn't great for tests and it lacked a bit of context on what was being tested so I converted them to cram tests. It's a bit harder to read the generated code than it was but the whole setup is not spread across 3 different directories anymore and the tests are now documented.

In their previous form they were not executed as part of the default
build or as part of the runtest alias.

They relied on a deriving_inline workflow which wasn't great for tests
and it lacked a bit of context on what was being tested so I converted
them to cram tests. It's a bit harder to read the generated code than
it was but the whole setup is not spread across 3 different directories
anymore and the tests are now documented.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb NathanReb added the no changelog Use this label to disable the changelog check github action label May 28, 2024
@NathanReb
Copy link
Collaborator Author

I'm not sure what the two_do_warn deriver was there for, maybe to test how the driver behaved with a set of alternating derivers, some where warning must be silenced and some where it must not.

CC @ceastlund maybe you know a bit more about this.

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already much clearer than the previous ml and dune setup for these tests! The naming of the flag I find a little confusing, but I think this is a good change regardless of any of the small documentation suggestions I have :))

test/deriving_warning/run.t Show resolved Hide resolved
test/deriving_warning/run.t Show resolved Hide resolved
@NathanReb NathanReb requested a review from patricoferris June 4, 2024 08:23
Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! The test documentation is good and I agree we can consolidate on some good documentation once other flags are added.

@NathanReb NathanReb merged commit a0d7c8e into ocaml-ppx:main Jun 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Use this label to disable the changelog check github action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants