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

Use ppx_inline_test for unit tests #623

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Conversation

ineol
Copy link
Collaborator

@ineol ineol commented Nov 24, 2023

As suggested by @ejgallego in #613, use inline tests instead of alcotest.

Copy link
Owner

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

Amazing, thanks so much!

Nix CI seems to need a tweak, I'll have a look later, but please feel free to merge yourself if you find a solution first.

@Alizter
Copy link
Collaborator

Alizter commented Nov 24, 2023

Nix is easy, just replace the alcotest in flake.nix with ppx_inline_test.

@ejgallego
Copy link
Owner

Nix is easy, just replace the alcotest in flake.nix with ppx_inline_test.

The PR seems to do that already, that's why I'm confused actually.

@Alizter
Copy link
Collaborator

Alizter commented Nov 24, 2023

Need to add it here too

inherit (ocamlPackages) yojson cmdliner uri dune-build-info;

It was only added to the devShell but you have to add it to the actual package that gets built too.

@ejgallego
Copy link
Owner

ejgallego commented Nov 24, 2023

I see, we are not running the tests on Nix, that's why the previous PR worked.

Comment on lines +6 to +7
(inline_tests)
(preprocess (pps ppx_inline_test))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This adds the test library as a dependency to the executable, which is probably not what you want. I think it would be better to have a separate test library like before.

Copy link
Owner

Choose a reason for hiding this comment

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

Does it? I think dune handles this properly.

@Alizter
Copy link
Collaborator

Alizter commented Nov 24, 2023

We shouldn't be adding test libraries as build dependencies anyhow. They should live in a separate library as they currently do with alcotest.

@ejgallego
Copy link
Owner

ejgallego commented Nov 24, 2023

We shouldn't be adding test libraries as build dependencies anyhow. They should live in a separate library as they currently do with alcotest.

Why? The whole idea of inline tests is to have the code next to the libraries, it is true that some people use a policy of splitting test libs , but so far this is a matter of taste.

Myself I'm fine with the tests living next to the use.

See official dune docs https://dune.readthedocs.io/en/stable/tests.html#inline-tests

@Alizter
Copy link
Collaborator

Alizter commented Nov 24, 2023

@ejgallego Yes, but you are adding another build dependency just so you are aware.

@ineol ineol merged commit 9a96682 into ejgallego:main Nov 24, 2023
12 checks passed
@ejgallego
Copy link
Owner

@ejgallego Yes, but you are adding another build dependency just so you are aware.

Yes; as it was the way it was done, I suggest we keep it this way.

But indeed if we find a problem with the new setup, we can just do a follow-up PR and split the tests to their own file.

I like (it's a personal preference, of course) when tests (and proofs) are near their corresponding definitions.

But indeed, an extra dep is a non trivial cost to pay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants