Skip to content

Add ppx_expect as a dependency #245

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 18, 2022
Merged

Add ppx_expect as a dependency #245

merged 1 commit into from
Oct 18, 2022

Conversation

@Alizter
Copy link
Contributor

@Alizter Alizter commented Oct 17, 2022 •
edited
Loading

This lets us write inline expectation tests which is quite useful especially in a hard to test application like coqbot.

Depends on #244.

@Alizter Alizter mentioned this pull request Oct 17, 2022
Closed
@Zimmi48
Copy link
Member

Zimmi48 commented Oct 18, 2022

You read my mind!

Zimmi48
Zimmi48 reviewed Oct 18, 2022
View reviewed changes
Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

Can you add an example ppx_expect test somewhere appropriate? Some example I had in mind was in bot-components, when we generate JSON, like in the play_job function.

@Alizter
Copy link
Contributor Author

Alizter commented Oct 18, 2022

@Zimmi48 I have a PR coming up with an expectation test. I've implemented the list of failed jobs for the bench. For ppx_expect to work we need to also do #244 as searching for the env var during the test causes an exception (since GITHUB_PRIVATE_KEY is not set).

@Alizter Alizter mentioned this pull request Oct 18, 2022
Merged
@Alizter
Copy link
Contributor Author

Alizter commented Oct 18, 2022

Let's merge this then I can show the other patch with a test.

Zimmi48
Zimmi48 approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

Fine, let's admit that you have tested it and merge.

Note that your src/dune file is not correctly formatted according to dune build @fmt but no big deal if you want to merge as is.

@Alizter
Copy link
Contributor Author

Alizter commented Oct 18, 2022

@Zimmi48 I can reformat it quickly.

@Alizter
Add ppx_expect as a dependency
918aa51 
This lets us write inline expectation tests which is quite useful
especially in a hard to test application like coqbot.

Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: ffabb830-d589-4c18-bd42-7b9a4e9ac75d
@Alizter Alizter force-pushed the ps/rr/add_ppx_expect_as_a_dependency branch from 7024e14 to 918aa51 Compare October 18, 2022 15:45
@Alizter
Copy link
Contributor Author

Alizter commented Oct 18, 2022

dune fmt done, merging.

@Alizter Alizter merged commit ce42ca8 into rocq-prover:master Oct 18, 2022
@Alizter Alizter deleted the ps/rr/add_ppx_expect_as_a_dependency branch October 18, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zimmi48 Zimmi48 Zimmi48 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Alizter @Zimmi48