uncaught exception: Unix.EPERM unlink

[hannesm]

hello,

when running alcotest (1.2.1) on the most recent icalendar release (v0.1.3), I run into the following issue:

$ dune runte
        test alias test/runtest (exit 125)
(cd _build/default/test && ./test.exe)
test.exe: internal error, uncaught exception:
          Unix.Unix_error(Unix.EPERM, "unlink", "/usr/home/hannes/devel/mirage/icalendar/_build/default/test/_build/_tests/")
          Raised by primitive operation at file "src/alcotest/alcotest.ml", line 40, characters 10-26
          Called from file "src/alcotest/alcotest.ml", line 69, characters 8-33
          Called from file "src/alcotest-engine/core.ml", line 348, characters 4-104
          Called from file "src/alcotest-engine/core.ml", line 447, characters 4-365
          Called from file "src/alcotest-engine/cli.ml", line 117, characters 4-128
          Called from file "cmdliner_term.ml", line 25, characters 19-24
          Called from file "cmdliner.ml", line 117, characters 32-39
Testing `'.
This run has ID `23AD5240-A1C0-486A-86C0-5E64DE247749'.

The issue is resolved by:

diff --git a/test/test.ml b/test/test.ml
index 0bf0f7b..7372d3b 100644
--- a/test/test.ml
+++ b/test/test.ml
@@ -2813,4 +2813,4 @@ let tests = [
 
 let () =
   Printexc.record_backtrace true;
-  Alcotest.run "" tests
+  Alcotest.run "icalendar suite" tests

I thought it is worth reporting, eventually you'd like to ensure that the suite named "" does not entail any issues (it was not an issue on a previous run, i.e. an earlier version of alcotest - take a look at the travis logs of roburio/icalendar if you want to figure out which alcotest version worked previously).

[craigfe]

Thanks for the report. You're right, we should have empty suite names and empty test names in our own test suite.

As far as resolving this issue goes, I'm tempted to say that empty suite names and test names should be unsupported (and raise a much nicer error message). They cause issues for both the console output and the log capturing mechanism, as shown here. What do you think?

[hannesm]

@craigfe I don't have a particular opinion on this, since my mental model of allowed test and suite names is rather brittle (I remember there were changes disallowing the same test / suite name twice, and something in respect to unicode, plus file path special characters (/, ..)).

I think it is fine to behave in this case the same as "use a test name twice", which AFAIR these days raise an exception.

[craigfe]

Test / suite name support has been changing quite a bit lately as we uncover more flaws with the design. At the moment:

• All UTF-8 strings are intended to be supported (apart from the empty string, it seems). We now use an escaping mechanism to avoid issues with non-FS-safe characters during log capturing.
• Test names cannot collide, as otherwise their on-disk logs would also collide.

I think it is fine to behave in this case the same as "use a test name twice", which AFAIR these days raise an exception.

Great. I'll make a fix for that shortly.