ocamlformat
ocamlformat copied to clipboard
ocaml-ppx
Add automated testing
We need a way to make sure no regressions are introduced. see what ocp-indent does: https://github.com/OCamlPro/ocp-indent/tree/rewind/tests
Totally agree that testing is needed. After some discussion with @let-def and some further digging, there are at least two systematic testing approaches that look like they would be useful here:
-
Use the incremental API of menhir to obtain a collection of token streams which drive the parser to each state of the automaton (not just the reduced error states, as the error handling mechanism does), and then convert those token streams to parsetrees, and finally check that ocamlformat correctly formats those parsetrees.
-
Fuzz ocamlformat using AFL via crowbar with parsetrees generated by @yomimono's ocaml-test-omp example for ppx_deriving_crowbar. In this case, there may be a complication due to the possibility of generating a parsetree which violates some invariants, causing the 'ast = parse (format ast)' check to fail in a way indistinguishable from an ocamlformat bug.
It would be very helpful to gather some input about the viability / utility / etc. of these approaches.
I've done some additional experiments with generating ASTs that don't violate quite so many invariants (and doesn't spend so much time changing constants) - there's some (very gory) code at https://github.com/yomimono/fuzz-experiments-boring/blob/4.06/test/parsetree_406.ml , which may or may not be useful in exploring option 2. In addition to customizing a bunch of generators, it also tries to parse generated ASTs before using them to form any conclusions about what works or doesn't.
I think it would be a good idea if the regression tests would also tests that ocp-indent and ocamlformat agrees on the file indentation. One way to ensure it remains consistent would be to use ocp-indent as a library and let it take care of indentation while ocamlformat will worry about alignement and line breaks. Or ocamlformat could call ocp-indent. Or something else :-)
It would be good to test that, if ocamlformat was designed with an attempt to mimic the indentation of ocp-indent. The basic implementation approaches are very different, and it would be highly nontrivial to ensure that the two are in sync. Running ocp-indent as a post-processing pass would likely wreak havoc on the relative indentation that ocamlformat thinks it is generating.
I'm also not sure that ocamlformat should even try to mimic a tool that doesn't have access to the full abstract syntax tree. But I'm probably not entirely objective. :-)
The goal is just to help adoption: a lot of care has been taken in ocp-indent to "please" the eyes of OCaml users; I think it would be a shame to just throw that away :-)
But yes, It's certainly not perfect and could be improved in some corner-cases by the knowledge of the full AST, but I think it's worth trying to minimise the diff between their outputs. For instance, if you run ocp-indent -i test/passing/*.ml there are a few examples where ocamlformat is indeed better, but it shows some cases which can be improved:
diff --git a/test/passing/list.ml b/test/passing/list.ml
index fb41fba..65ccc7d 100644
--- a/test/passing/list.ml
+++ b/test/passing/list.ml
@@ -5,9 +5,9 @@ let f x =
with
| P
({xxxxxxxxxxxxxxxxxxxxxx}
- :: {yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy}
- :: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz)
- -> true
+ :: {yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy}
+ :: zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz)
+ -> true
diff --git a/test/passing/gadt.ml b/test/passing/gadt.ml
index 70f8537..3498436 100644
diff --git a/test/passing/gadt.ml b/test/passing/gadt.ml
index 70f8537..3498436 100644
--- a/test/passing/gadt.ml
+++ b/test/passing/gadt.ml
@@ -6,8 +6,8 @@ type (_, _, _, _, _) gadt =
| SomeLongName:
('a, 'b, long_name * long_name2, 't, 'u) gadt
* ('b, 'c, 'v, 'u, 'k) gadt2
- -> ('a, 'c, long_name * 'k, 't, 'v) gadt
+ -> ('a, 'c, long_name * 'k, 't, 'v) gadt
| AnEvenLongerName:
('a, 'b, long_name * long_name2, 't, 'u) gadt
* ('b, 'c, 'v, 'u, 'k) gadt2
- -> ('a, 'c, long_name * 'k, 't, 'v) gadt
+ -> ('a, 'c, long_name * 'k, 't, 'v) gadt
And a few lines ends by extra whitespaces.
So the full integration between the two projects is indeed not necessary (and " non-trivial" amount of work) but I think it's still worth it trying not to diverge too much :-)
To give another data-point, having the output of of ocamlformat matches ocp-indent --config=JaneStreet is a agreed upon requirement before considering opting in into ocamlformat on the files I am working on. To get going, ocp-indent is currently applied as a additional step. There are some corner cases (when ocamlformat indents less, some lines end up exceeding the margin after the indentation pass). I don't feel very strongly in term of priorities since that seems to work pretty well. Not having them match is in my mind only a technical limitation/difficulty, not a feature.
I think perhaps my reply didn't come across as intended, I'm sorry for that. I certainly agree that there is a ton of experience codified into ocp-indent, and don't want to throw it away. Not to mention that it has the killer capability where it works even when a file does not parse, so in particular is indispensable for use while editing a file.
I think that from the perspective of ocamlformat development, ocp-indent is an excellent point of reference. It certainly handles some cases better, and can be a very useful tool to point out deficiencies. But I am still very wary of making agreement with ocp-indent part of ocamlformat's specification. When not using ocamlformat, I mostly but not always agree with ocp-indent, and I'm not sure that I'm ready to give up the flexibility to do things differently, in particular given the additional information in the parsetree. (I also suspect that I at least have been trained to some extent by ocp-indent to expect what it does, just because that is what it does and most code I read has been indented with it.)
It would be informative to track the degree of discrepancy between the two, I'm just not sure I would call the differences "bugs".
Post-processing with ocp-indent sounds like a reasonable approach, as long as overshooting the margin a bit is not too bad (or the margin ocamlformat aims for is reduced a bit to compensate). I worry that there are cases where ocp-indent adds significantly more indentation. Have any such cases been encountered?
Have any such cases been encountered?
Yes. While the following is quite reasonably printed by ocamlformat:
let a_longish_function_name (type a) (type comparator_witness) (module S
: Module_type_that_passes_the_margin with type t = a and type comparator_witness = comparator_witness)
=
()
;;
the extra pass to ocp-indent result in a very long line:
let a_longish_function_name (type a) (type comparator_witness) (module S
: Module_type_that_passes_the_margin with type t = a and type comparator_witness = comparator_witness)
=
()
;;
Ouch. But I'm not personally all that happy with the ocamlformat output in this case.
https://github.com/aantron/bisect_ppx could help us measure how good (bad really) the regtests are.
Retitled as regression testing has been in place for a while, but more thorough automated testing remains.
For this one we have to wait for https://github.com/ocaml-ppx/ppx_import/pull/27 to be merged
@emillon, it seems that you have a workflow for automated testing (issues with the fuzz label). Is it documented anywhere ?
It's not documented yet (will be soon) but you can have a look at this dir in my fork in the meantime. The idea is to generate random ASTs with crowbar and find examples that parse but trigger formatting errors.
@emillon is that something that is ready to be merged so we can close this issue?
The issue itself is still completely relevant, even if the implementation strategy explored so far hasn't gone the whole way.
JFYI, we should be able to generate from Menhir grammar list of sentences to cover tricky cases (for instance all automaton transitions involved in a conflict).
