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

Add ppx tests setup #118

Merged
merged 9 commits into from
Oct 16, 2020
Merged

Add ppx tests setup #118

merged 9 commits into from
Oct 16, 2020

Conversation

jchavarri
Copy link
Contributor

Includes a minimal setup of "snapshot"-like tests for the ppx. This kind of tests have been really helpful to me, not only to know when the output of the ppx is changing and how, but also as documentation.

I just added a couple of tests for now to cover the issues mentioned in #116 and #117, in order to ask for feedback. If this sounds like a good idea, maybe the content of test_bindings.mli, or a subset of it, could be added to input.ml.

@jchavarri jchavarri marked this pull request as ready for review October 14, 2020 21:12
@mlasson
Copy link
Member

mlasson commented Oct 15, 2020

Good idea !
I added some tests and found some bugs.
This will be convenient to avoid regression while migrating to ppxlib.

Copy link
Contributor Author

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Looks great @mlasson ! Good to hear it'll be useful with ppxlib migration :)

Should the binding_*.mli files include an example with [@js.scope]? Or do you prefer to add it after #117 is fixed?

@@ -0,0 +1,7 @@
module Issue116 = [%js: type t]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

@mlasson mlasson merged commit 6e6d116 into LexiFi:master Oct 16, 2020
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