Some expect tests?

[Lupus]

I'm thinking that some expect tests should help with maintaining the ocaml-protoc codebase with less effort. At the cost of additional dependency, say ppx_yojson_conv, we could instrument all of internal representations with yojson_of_t, and use ppx_expect for tests. You just need to feed a bunch of proto definitions in a form of strings into library bits, get some structures as output, pretty-print them as JSON and ppx_expect will capture them and check that code works in the same way it used to without any hand-writing of unit tests. In my experience expect tests with automatic pretty-printing of internal structures works incredibly well with compiler-kind of projects, which ocaml-protoc definitely belongs to.

[c-cube]

We don't need a ppx I think, we already have printers! Dune expect tests are fairly good already. But I think it's a good idea!

[Lupus]

Dune expect tests are quite heavyweight compared to ppx_expect, with latter you can have dozens of granular tests in one source file, while with dune expect test you can have only one test output per test, and to have multiple tests you need to create multiple dune rules and that gets pretty verbose. ppx_expect won't become a runtime dependency, tests are easily organized into separate lib. Or you want to have minimal dependency footprint for the library and thus suggest to use dune, which is already a dependency?

[c-cube]

You can have multiple outputs per file, they'll just be concatenated. You can separate them with a printf if you want but there's no inherent need.

And yeah I'd rather avoid heavyweight dependencies even if it's just for testing.

[Lupus]

We don't need a ppx

I'm currently adding some support for missing cases with option parsing. Looking at current tests with looots of manual asserts for all the fields spelled out by hand, I feel very reluctant to write any tests for my changes. There are no printers for compilerlib/pb_parsing_parse_tree.ml or compilerlib/pb_typing_type_tree.ml, I can't just print the stuff and match it with expected output, via dune or ppx_expect...

I wonder how many contributions in OCaml community were never completed/upstreamed because of "don't bring new dependencies! it's OCaml, we don't have decent stdlib, nor runtime type information/standard derivers, you have to code everything by hand in every project, only hardcore!" principle... </rant> sorry, I'm too accustomed to our corporate environment where most of the tests are actually expect tests and all the code to print stuff is written by a machine, it's literally zero friction to maintain tests along with new features.

[mbarbin]

Hello @Lupus, @c-cube, and team,

I've been following this discussion and wanted to share my thoughts and a proposal.

Firstly, I agree with @Lupus's point about the potential benefits of expect tests in maintaining the ocaml-protoc codebase. I've found that expect tests, particularly when combined with automatic pretty-printing of internal structures, can be incredibly effective for compiler-like projects such as this one.

I also understand @c-cube's concerns about introducing heavyweight dependencies and the desire to keep the dependency footprint minimal. I believe there's a way to balance these considerations, as far as testing the Ocaml generated code.

In my recent PR #229, I made a small change that affects the generated code. I noticed that this didn't yield any corresponding changes in the tests. This led me to think about how we could improve the testing workflow.

Here's a proposal: For every ml and mli file generated by a dune rule in the src/examples/ directory, we add a corresponding *.ml.expect and *.mli.expect file. We could then create a dune rule that diffs the generated files against their expected reference as part of dune @runtest.

This approach would allow us to easily see the impact of every PR on the generated code across all examples. It would also provide a proof that the generated code didn't change for every change that isn't expected to have an impact on it (such as refactors, etc.)

I believe this wouldn't add too much maintenance pressure as updating the tests would simply involve running dune promote. Moreover, it would make the testing process more robust and transparent, which could potentially encourage more contributions.

If this proposal aligns with your vision for the project, I'd be happy to write a PR that adds these dune rules. This could serve as a prerequisite to my upcoming contribution.

Looking forward to hearing your thoughts. Thank you!

[c-cube]

Short reply from my phone.

First, improving the testing workflow would be great. Right now it involves fetching a submodule and having protoc in the path, and it's a bit crufty. There are some expect tests already but not for the rpc part. More testing is always good, and I'm open to some testing dependencies as long as it doesn't affect the non testing code (so a expect ppx would be fine if it's not used in the libraries themselves; I find dune cramtests to be enough personally though).

Anyway, thank you for offering to do the hard work! 😁

[Lupus]

#230 looks really nice! Great work @mbarbin!

[mbarbin]

Thanks for your kind words on #230, @Lupus! On a related note, I'm discussing roundtrip property testing for ocaml-protoc here: #233. Your input would be greatly appreciated. Thank you!