exercism / ocaml

Exercism exercises in OCaml.
https://exercism.org/tracks/ocaml
MIT License
93 stars 50 forks source link

Rearchitect test generator #361

Closed marionebl closed 5 years ago

marionebl commented 5 years ago
sshine commented 5 years ago

Does this PR leave anything missing in #284?

marionebl commented 5 years ago

Does this PR leave anything missing in #284?

This adds versioning to all exercises that generate tests from problem-specifications. Exceptions:

Also the new test_gen does not create .meta/version files yet

sshine commented 5 years ago

Since this PR doesn't add .meta/version to the generator, I wonder if we should leave this as a "good first issue" for the upcoming hacktoberfest. I think this would be very approachable considering how much you've modernized the test generator already.

sshine commented 5 years ago

I've updated #284 to reflect the changes this PR makes and what's still missing wrt. versioning.

sshine commented 5 years ago

I've removed this point and re-inserted it here:

For the sake of merging soon, the only thing I'd like to have addressed is that I am not sure why .editorconfig was added to this PR. What does it have to do with the test generator?

marionebl commented 5 years ago

I've removed this point and re-inserted it here:

For the sake of merging soon, the only thing I'd like to have addressed is that I am not sure why .editorconfig was added to this PR. What does it have to do with the test generator?

Ah I didn't catch this. I added .editorconfig as a semi-automated way to address the multitude of trailing whitespace issues that popped up in the review. Also I had the lofty idea to write a editorconfig implementation in OCaml and integrate it into test_gen. After looking into the complexity of existing implementations I backed of from that part of the plan (for now).

marionebl commented 5 years ago

I see that your current solution wrt. having both generated oUnit and manually written QCheck is to add QCheck exercises to the test.ml.tpl template file [...]

I think this is very acceptable; [...] I wonder if we should consider [...] whether to document this part of the test generator.

Yes, let's document the updated mechanics of test_gen. I suspect we'll find some opportunities to make the project layout a bit easier to understand while documenting - I ran into mental overhead mainly caused by having to relate the source files in templates, exercises and problem-specifications/exercises while working on a single exercise.

I repeatedly thought it might be easier to have all sources reside in templates and use exercises as "artifact storage" exclusively.

I don't recall if you have made any CI checks that run the test generator and sees if there is a discrepancy? Providing a meaningful CI error would be very useful if contributors in the future think they can address the generated test suites directly, and it also asserts that the test generator can be built.

Yes, those checks are in place:

1. Consistency check: https://github.com/exercism/ocaml/blob/3d241631079ed6814065527dd03105df555e6d31/.travis/.travis-ci.sh#L18-L24

**2. test_gen run https://github.com/exercism/ocaml/blob/3d241631079ed6814065527dd03105df555e6d31/.travis/.travis-ci.sh#L13

sshine commented 5 years ago

Hooray!