-
Notifications
You must be signed in to change notification settings - Fork 234
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
Implement a Fuzzy CI to catch ocamlmerlin
regressions
#1716
Conversation
Btw, the CI is expected to fail on this PR: The first commit changes the |
1aacfd4
to
5924649
Compare
31a334c
to
cbc4daa
Compare
8ed7d3c
to
c1c8175
Compare
This PR changes the response of some of the
|
c1c8175
to
1205048
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also discuss whether we want to reduce the number of query types we test. Currently, it's
type-enclosing
,occurrences
,locate
,complete-prefix
, anderrors
. I think all of them are useful to test. What do you think?
I think that's a good starting point.
I made a review pass on the PR. My main worry is future maintenance: the actions are quite complex. Do you know what, in day-to-day Merlin work might require changes to the CI ? I guess upgrading to a new version of OCaml is one, but are there other things we should take in consideration?
--sample-size=30 \ | ||
--data=${{ env.data_dir }} \ | ||
--merlin=ocamlmerlin \ | ||
--project=irmin/src/irmin,irmin/src/irmin-pack,irmin/test/irmin-pack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could take advantage of the monorepo to also run on more diverse projects and not only Irmin ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, adding more diverse projects to the workflow should be relatively easy (I'd then decrease the number of samples per file, though, to avoid having the CI run for too long). Do you have projects in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would target something that is part of Irmin's dependencies but didn't have a look yet (and probably won't before next year...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to have a look if anything in the Irmin dependency cone adds additional value to the test base. As mentioned in the comment about the CI execution time, it's better for the execution time to have few files with lots of samples than to have lots of files with few samples.
(and probably won't before next year...).
(same for me :) )
I'd say that there are the following main areas of attention in "day-to-day" Merlin work:
Like every CI, there are also a few Merlin-external factors that might need maintenance:
|
I've just updated the comment above, crossing out one maintenance point (after implementing a fix). I think maintenance of this CI would be quite ok, but do let me know if you think differently! The main maintenance burden (I hope) will be updating the CI when Merlin bumps the compiler. As you've mentioned, that happens only twice a year. I'm also very happy to update the CI myself those two times per year. What will need to be done will be to edit an env variable in the CI and to generate a lock file for the test code base Irmin. If you think similarly as me about maintenance, this might be ready to be merged. |
Thanks a lot Sonja ! Let's merge it and see how it goes :-) |
Could rebase and squash commits into relevant steps ? |
This commit adds an iterator called query_num to the ocamlmerlin output. The motivation for that is to identify server crashes (the server has crashed iff the counter is back to 0).
a89039a
to
38ee345
Compare
This commit implements a Fuzzy CI to catch end-to-end regressions in Merlin by checking the responses of a set of ocamlmerlin queries. That set of queries is determined by a deterministic but randomly chosen set of file location samples on the Irmin code base. The CI passes automatically if the responses on all samples remain the same before and after the PR. If there's a diff in the responses, the CI fails and makes the diff available as a GH action artifact. There's a high-level description of the CI in much more detail on https://github.com/ocaml/merlin/wiki/Merlin-Fuzzy-CI. Co-authored-by: Enguerrand Decorne <[email protected]>
38ee345
to
842515e
Compare
Cool, thanks! I've just squashed the commits. As a heads-up: I'm planning to transfer the |
If your confident this will happen during the next few weeks then let's wait for it 🙂 |
In the end, I'm not prioritizing transferring |
CHANGES: Thu Feb 22 14:00:42 CET 2024 + merlin binary - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717) - Add cache stats to telemetry (ocaml/merlin#1711) - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706) - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722) - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716) - Jump to cases within a match statement (ocaml/merlin#1726) - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656) - Exposes stable functions for configuration handling and pattern variable destruction. (ocaml/merlin#1730) + editor modes - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340) - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
CHANGES: Thu Feb 22 14:00:42 CET 2024 + merlin binary - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717) - Add cache stats to telemetry (ocaml/merlin#1711) - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706) - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722) - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716) - Jump to cases within a match statement (ocaml/merlin#1726) - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656) - Exposes stable functions for configuration handling and pattern variable destruction. (ocaml/merlin#1730) + editor modes - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340) - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
CHANGES: Thu Feb 22 14:00:42 CET 2024 + merlin binary - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717) - Add cache stats to telemetry (ocaml/merlin#1711) - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706) - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722) - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716) - Jump to cases within a match statement (ocaml/merlin#1726) - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656) - Exposes stable functions for configuration handling and pattern variable destruction. (ocaml/merlin#1730) + editor modes - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340) - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
CHANGES: Thu Feb 22 14:00:42 CET 2024 + merlin binary - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717) - Add cache stats to telemetry (ocaml/merlin#1711) - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706) - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722) - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716) - Jump to cases within a match statement (ocaml/merlin#1726) - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656) - Exposes stable functions for configuration handling and pattern variable destruction. (ocaml/merlin#1730) + editor modes - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340) - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
This PR implements a Fuzzy CI, that catches end-to-end regressions in Merlin by checking the responses of a fixed set of
ocamlmerlin
queries. That set of queries is determined by a deterministic but randomly chosen set of file location samples on the Irmin code base. The CI passes automatically if the responses on all samples remain the same before and after the PR. If there's a diff in the responses, the CI fails and makes the diff available as a GH action artifact. There's a high-level description of the CI in much more detail on a new wiki page about the CI.Motivation for the CI
Merlin already has quite a few end-to-end tests (which are also run in CI). All of those are behavior-specific: We want to test one concrete behavior and write a tailored test for it. Additionally to those end-to-end tests, we now also want to test non-specifically on a large randomized input set.
Some implementation details
There are two reasons why the data of the
ocamlmerlin
responses is generated in the CI instead of living in the repo: 1. The data is over 20 MB big. 2. The generation of the data takes over 10 minutes, and we don't want our contributors to have to re-generate it for PRs that change theocamlmerlin
responses.With the current implementation, the CI takes about 14-15 min to run. That's about 2 min longer than the mac-os CI (edit: Actually nvm, the mac-os CI is only one job of the
main.yml
workflow, which in total takes over 20 min, so clearly longer than this workflow! :)), which is the currently longest CI. There'd be a straightforward way to reduce the running time of this CI by another ~30 seconds. Let me know if I should do that.I also think that we should observe over the coming months if a given
ocamlmerlin
response change will always be duplicated among multiple samples. If that's the case, we can reduce the sample size, which would speed up the CI.We can also discuss whether we want to reduce the number of query types we test. Currently, it's
type-enclosing
,occurrences
,locate
,complete-prefix
, anderrors
. I think all of them are useful to test. What do you think?