Skip to content
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

reference tests for easycrypt extraction #863

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

cassiersg
Copy link
Collaborator

@cassiersg cassiersg commented Jul 11, 2024

Tests that the easycrypt extraction matches vested "golden" files, up to insignificant whitespace changes.

@cassiersg cassiersg changed the title [WIP] reference tests for easycrypt extraction reference tests for easycrypt extraction Jul 11, 2024
@cassiersg cassiersg requested a review from eponier July 11, 2024 11:04
@cassiersg
Copy link
Collaborator Author

@vbgl Do you know why the CI fails?

I see that https://gitlab.com/jasmin-lang/jasmin/-/jobs/7317501176 fails because of No such file or directory: './scripts/extract-and-check'.

Running nix-shell --run 'make -C compiler CHECKCATS="x86-64-extraction arm-m4-extraction" check' locally works.

- Do not clutter the compiler/ directory with temp files: the test
  results are now in deterministic locations, in a file hierarchy under
  compiler/tests/results
- Tell easycrypt to use eclib from this repo.
@vbgl
Copy link
Member

vbgl commented Jul 11, 2024

Is it really needed to store so many expected test results? Shouldn’t a checksum be enough to detect regressions?

@cassiersg
Copy link
Collaborator Author

That could be done indeed, but w'd loose:

  1. the current fine-grained approach where we can ignore non-significant white-space changes
  2. ease of reviewing what changes: currently, if the test fails, it displays a diff expected vs new. (Both when promoting the new reference files as a reviewer, and when reviewing the changes as a PR reviewer.)

It is debatable whether 1. is a good idea to have. For 2., I think that it's a significant annoyance.

The long-term plan of @eponier is to move this to dune "cramtest" (and extend it to also check assembly output).

We could also try to reduce the number of files by putting all the test results in a single file (tar-like, but in a human-readable format). Would that help to adress your concerns ? Is the issue more w.r.t. to the number of files, or with the amount of data in the repository ?

@vbgl
Copy link
Member

vbgl commented Jul 16, 2024

I’m mostly concerned by the amount of data that is added there.

@cassiersg
Copy link
Collaborator Author

(To quantify: it's about 6.8 MB of data.)

I have no strong opinion (and I am not a maintainer), so I'm totally open to moving to hashes. We can always change the approach later if point 2. above ends up being too annoying. Should I move forward with a solution with hashes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants