search

mirage / ocaml-tar

Pure OCaml library to read and write tar files
ISC License
54 stars 34 forks source link

Replace ounit2 with Alcotest #121

Closed MisterDA closed 1 year ago

MisterDA commented 1 year ago

Alcotest has better error reporting.

MisterDA commented 1 year ago

A thing I struggled a bit with before is when running tests and they fail it can be useful to keep temporary files / the archive in order to inspect what went wrong, but it didn't seem too easy to do with ounit2. Do you have ideas how we could improve this? Doesn't have to be part of this PR.

Can we do this if instead of cleaning files in finalizers, we just clean files after the test? If an assertion fails, the code won't advance until the cleaning part.

I don't understand yet why the Windows tests are failing. They can't remove the tar files behind them.

reynir commented 1 year ago

This looks good to me. I can't figure out why the windows CI fails.

MisterDA commented 1 year ago

Let me test on Windows locally. I'll report shortly.

MisterDA commented 1 year ago

Can't reproduce the new Windows failure locally, but I've fixed the previous ones.

reynir commented 1 year ago

I pushed a fix for something which I believe is broken behavior in the test. The documentation for Tar.Make.Archive.with_next_file says:

(** Read the next header, apply the function 'f' to the fd and the header. The function
    should leave the fd positioned immediately after the datablock. Finally the function
    skips past the zero padding to the next header *)

We do not skip anything in the test. It seems somewhat brittle to expect the function to position the reader at the end of the data.

reynir commented 1 year ago

It seems the test passes now :shrug:

Edit: except I broke it again oops

Edit2: no, they weren't passing and still aren't :/

MisterDA commented 1 year ago

Must be transient failures, because now it's succeeding! Thanks for all the help!