Update letop-punning tests, add note to CONTRIBUTING (#2748)

WardBrian · web-flow · commit 7ec4b50cb8aa · 2025-11-05T16:00:08.000+01:00
* Fix redundancy in letop-punning tests * Notes on adding a test in CONTRIBUTING * CHANGES entry * Edits per @EmileTrotignon * Typo repair * Edits per @Julow
diff --git a/CHANGES.md b/CHANGES.md
@@ -36,6 +36,9 @@ profile. This started with version 0.26.0.
   end
   ```
 
+### Internal
+
+- Added information on writing tests to `CONTRIBUTING.md` (#2838, @WardBrian)
 
 ### Changed
 
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -19,8 +19,57 @@ We heartily welcome pull requests. To ensure an effective contribution, please a
 1. For significant, invasive, or output-affecting changes, consider opening an issue for discussion before committing substantial time and effort.
 2. If you're new to the project, starting with the [Good first issues](https://github.com/ocaml-ppx/ocamlformat/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+label%3A%22Good-first-issue+%3Agreen_heart%3A%22) can be beneficial.
 3. Fork the repository and create your branch from `main`.
-4. If you've added code that should be tested, supplement it with tests located in the `test/` directory.
-5. Ensure that the test suite passes (see [Running the tests](#running-the-tests) for instructions).
+4. Ensure that the test suite passes (see [Running the tests](#running-the-tests) for instructions). If there are changes in the tests,
+   verify that they are expected. If no existing test output changed, you likely need to add new tests (see [Adding a test](#adding-a-test)).
+
+### Adding a Test
+
+#### Golden Tests
+
+The majority of tests for `ocamlformat` are so called "golden" or "expect" tests. These
+tests all consist of running `ocamlformat` on `.ml` files and comparing the
+output to expected output stored in reference files. The `dune` configuration
+for this is automatically generated for you based on the files present in these
+directories the first time `dune runtest` is executed.
+
+These are separated into `test/failing` and `test/passing` directories.
+
+To add a test showing currently incorrect behavior, add a `.ml` file to
+`test/failing/tests`. If command line arguments are needed, create a
+corresponding `.ml.opts` file with the same name. The output of these tests
+will be stored in `.ml.broken-ref` files.
+
+To add a test showing correct behavior, add a `.ml` file to `test/passing/tests`
+or update an existing file there. These tests are similar to the failing tests,
+including the use of the `.ml.opts` files for command line arguments, however the
+same file will be tested against all of the built-in configurations (e.g.
+`default`, `janestreet`, etc.), with outputs appearing in sub folders named
+`refs.<configuration>`.
+
+In both cases, if multiple sets of options are desired, you can create multiple
+`.ml.opts` files with names provided after a `-`, for example `mytest.ml` can have
+`mytest-foo.ml.opts` and `mytest-bar.ml.opts`. This will create multiple test targets.
+
+The first time you [run the tests](#running-the-tests), the `dune.inc` file will
+be updated. Once you have promoted this, subsequent runs will show you any
+differences between the expected and actual outputs, which can be accepted with
+`dune promote`.
+
+#### cram-Style Tests
+
+The remaining tests are not designed to test the formatting output of
+`ocamlformat`, but rather the command line interface behavior, such as error
+messages and help text.
+
+These are found in `test/cli` and are written using
+[cram testing](https://dune.readthedocs.io/en/latest/reference/cram.html).
+Each test is a `.t` file that contains shell commands and their expected
+outputs. To add a new test, create a new `.t` file, or add new commands (lines
+beginning with ` $`) to an existing file.
+
+#### Unit Tests
+
+Unit tests are located in the `test/unit` directory and are written using [Alcotest](https://github.com/mirage/alcotest).
 
 ### Running the Tests
 
diff --git a/test/failing/dune.inc b/test/failing/dune.inc
@@ -181,6 +181,19 @@
  (package ocamlformat)
  (action (diff tests/js_test.ml.broken-ref js_test.ml.output)))
 
+(rule
+ (deps tests/.ocamlformat )
+ (package ocamlformat)
+ (action
+   (with-outputs-to letop_broken.ml.output
+     (with-accepted-exit-codes 1
+       (run %{bin:ocamlformat} --ocaml-version=4.14 --letop-punning=always %{dep:tests/letop_broken.ml})))))
+
+(rule
+ (alias runtest)
+ (package ocamlformat)
+ (action (diff tests/letop_broken.ml.broken-ref letop_broken.ml.output)))
+
 (rule
  (deps tests/.ocamlformat )
  (package ocamlformat)
diff --git a/test/failing/tests/letop_broken.ml b/test/failing/tests/letop_broken.ml
@@ -0,0 +1,18 @@
+let ( let* ) x f = f x
+
+let ( and* ) a b = (a, b)
+
+let x = 1
+
+and y = 2
+
+and z = 3
+
+
+
+let r = let* (* 1 *) x (* 2 *) = (* 3 *) x (* 4 *)
+and* (* 5 *) y (* 6 *) in (x, y)
+
+let s = let%foo (* 1 *) x (* 2 *) = (* 3 *) x (* 4 *)
+and (* 5 *) y (* 6 *) in (x, y)
+
diff --git a/test/failing/tests/letop_broken.ml.broken-ref b/test/failing/tests/letop_broken.ml.broken-ref
@@ -0,0 +1,8 @@
+ocamlformat: Cannot process "tests/letop_broken.ml".
+  Please report this bug at https://github.com/ocaml-ppx/ocamlformat/issues.
+  BUG: comment changed.
+File "tests/letop_broken.ml", line 13, characters 33-40:
+Error: comment (*  3  *) dropped.
+  BUG: comment changed.
+File "tests/letop_broken.ml", line 13, characters 43-50:
+Error: comment (*  4  *) dropped.
diff --git a/test/failing/tests/letop_broken.ml.opts b/test/failing/tests/letop_broken.ml.opts
@@ -0,0 +1 @@
+--ocaml-version=4.14 --letop-punning=always
diff --git a/test/passing/gen/dune.inc b/test/passing/gen/dune.inc
@@ -3708,55 +3708,55 @@
  (deps .ocamlformat)
  (package ocamlformat)
  (action
-  (with-stdout-to let_punning.ml.stdout
-   (with-stderr-to let_punning.ml.stderr
-     (run %{bin:ocamlformat} --name let_punning.ml --margin-check %{dep:../tests/let_punning.ml})))))
+  (with-stdout-to let_punning-denied.ml.stdout
+   (with-stderr-to let_punning-denied.ml.stderr
+     (run %{bin:ocamlformat} --name let_punning-denied.ml --margin-check --letop-punning=never --ocaml-version=4.14.0 %{dep:../tests/let_punning.ml})))))
 
 (rule
  (alias runtest)
  (package ocamlformat)
- (action (diff let_punning.ml.ref let_punning.ml.stdout)))
+ (action (diff let_punning-denied.ml.ref let_punning-denied.ml.stdout)))
 
 (rule
  (alias runtest)
  (package ocamlformat)
- (action (diff let_punning.ml.err let_punning.ml.stderr)))
+ (action (diff let_punning-denied.ml.err let_punning-denied.ml.stderr)))
 
 (rule
  (deps .ocamlformat)
  (package ocamlformat)
  (action
-  (with-stdout-to let_punning_denied.ml.stdout
-   (with-stderr-to let_punning_denied.ml.stderr
-     (run %{bin:ocamlformat} --name let_punning_denied.ml --margin-check --letop-punning=never --ocaml-version=4.14.0 %{dep:../tests/let_punning_denied.ml})))))
+  (with-stdout-to let_punning-preferred.ml.stdout
+   (with-stderr-to let_punning-preferred.ml.stderr
+     (run %{bin:ocamlformat} --name let_punning-preferred.ml --margin-check --letop-punning=always --ocaml-version=4.14.0 %{dep:../tests/let_punning.ml})))))
 
 (rule
  (alias runtest)
  (package ocamlformat)
- (action (diff let_punning_denied.ml.ref let_punning_denied.ml.stdout)))
+ (action (diff let_punning-preferred.ml.ref let_punning-preferred.ml.stdout)))
 
 (rule
  (alias runtest)
  (package ocamlformat)
- (action (diff let_punning_denied.ml.err let_punning_denied.ml.stderr)))
+ (action (diff let_punning-preferred.ml.err let_punning-preferred.ml.stderr)))
 
 (rule
  (deps .ocamlformat)
  (package ocamlformat)
  (action
-  (with-stdout-to let_punning_preferred.ml.stdout
-   (with-stderr-to let_punning_preferred.ml.stderr
-     (run %{bin:ocamlformat} --name let_punning_preferred.ml --margin-check --letop-punning=always --ocaml-version=4.14.0 %{dep:../tests/let_punning_preferred.ml})))))
+  (with-stdout-to let_punning.ml.stdout
+   (with-stderr-to let_punning.ml.stderr
+     (run %{bin:ocamlformat} --name let_punning.ml --margin-check %{dep:../tests/let_punning.ml})))))
 
 (rule
  (alias runtest)
  (package ocamlformat)
- (action (diff let_punning_preferred.ml.ref let_punning_preferred.ml.stdout)))
+ (action (diff let_punning.ml.ref let_punning.ml.stdout)))
 
 (rule
  (alias runtest)
  (package ocamlformat)
- (action (diff let_punning_preferred.ml.err let_punning_preferred.ml.stderr)))
+ (action (diff let_punning.ml.err let_punning.ml.stderr)))
 
 (rule
  (deps .ocamlformat)
diff --git a/test/passing/refs.ahrefs/let_punning-denied.ml.ref b/test/passing/refs.ahrefs/let_punning-denied.ml.ref
@@ -15,7 +15,7 @@ let p =
   x, y, z
 
 let q =
-  let%foo x = x
+  let%foo x
   and y = y
-  and z = z in
+  and z in
   x, y, z
diff --git a/test/passing/refs.ahrefs/let_punning-preferred.ml.ref b/test/passing/refs.ahrefs/let_punning-preferred.ml.ref
@@ -15,7 +15,7 @@ let p =
   x, y, z
 
 let q =
-  let%foo x = x
+  let%foo x
   and y = y
-  and z = z in
+  and z in
   x, y, z
diff --git a/test/passing/refs.ahrefs/let_punning.ml.ref b/test/passing/refs.ahrefs/let_punning.ml.ref
@@ -9,13 +9,13 @@ and y = 2
 and z = 3
 
 let p =
-  let* x
+  let* x = x
   and* y
-  and* z in
+  and* z = z in
   x, y, z
 
 let q =
-  let%foo x = x
+  let%foo x
   and y = y
-  and z = z in
+  and z in
   x, y, z
diff --git a/test/passing/refs.default/let_punning-denied.ml.ref b/test/passing/refs.default/let_punning-denied.ml.ref
@@ -10,5 +10,5 @@ let p =
   (x, y, z)
 
 let q =
-  let%foo x = x and y = y and z = z in
+  let%foo x and y = y and z in
   (x, y, z)
diff --git a/test/passing/refs.default/let_punning-preferred.ml.ref b/test/passing/refs.default/let_punning-preferred.ml.ref
@@ -10,5 +10,5 @@ let p =
   (x, y, z)
 
 let q =
-  let%foo x = x and y = y and z = z in
+  let%foo x and y = y and z in
   (x, y, z)
diff --git a/test/passing/refs.janestreet/let_punning-denied.ml.ref b/test/passing/refs.janestreet/let_punning-denied.ml.ref
@@ -13,8 +13,8 @@ let p =
 ;;
 
 let q =
-  let%foo x = x
+  let%foo x
   and y = y
-  and z = z in
+  and z in
   x, y, z
 ;;
diff --git a/test/passing/refs.janestreet/let_punning-preferred.ml.ref b/test/passing/refs.janestreet/let_punning-preferred.ml.ref
@@ -13,8 +13,8 @@ let p =
 ;;
 
 let q =
-  let%foo x = x
+  let%foo x
   and y = y
-  and z = z in
+  and z in
   x, y, z
 ;;
diff --git a/test/passing/refs.ocamlformat/let_punning-denied.ml.ref b/test/passing/refs.ocamlformat/let_punning-denied.ml.ref
@@ -13,5 +13,5 @@ let p =
   (x, y, z)
 
 let q =
-  let%foo x = x and y = y and z = z in
+  let%foo x and y = y and z in
   (x, y, z)
diff --git a/test/passing/refs.ocamlformat/let_punning-preferred.ml.ref b/test/passing/refs.ocamlformat/let_punning-preferred.ml.ref
@@ -13,5 +13,5 @@ let p =
   (x, y, z)
 
 let q =
-  let%foo x = x and y = y and z = z in
+  let%foo x and y = y and z in
   (x, y, z)
diff --git a/test/passing/tests/let_punning-denied.ml.opts b/test/passing/tests/let_punning-denied.ml.opts
diff --git a/test/passing/tests/let_punning-preferred.ml.opts b/test/passing/tests/let_punning-preferred.ml.opts
diff --git a/test/passing/tests/let_punning.ml b/test/passing/tests/let_punning.ml
@@ -9,9 +9,9 @@ and y = 2
 and z = 3
 
 let p =
-  let* x and* y and* z in
+  let* x = x and* y and* z = z in
   (x, y, z)
 
 let q =
-  let%foo x = x and y = y and z = z in
+  let%foo x and y = y and z in
   (x, y, z)
diff --git a/test/passing/tests/let_punning_denied.ml b/test/passing/tests/let_punning_denied.ml
diff --git a/test/passing/tests/let_punning_preferred.ml b/test/passing/tests/let_punning_preferred.ml

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+--ocaml-version=4.14 --letop-punning=always`