multicoretests icon indicating copy to clipboard operation
multicoretests copied to clipboard

Add Sys.is_directory, Sys.remove and Sys.rename

Open jmid opened this issue 2 years ago • 8 comments

This PR adds Sys.is_directory, Sys.remove and Sys.rename to the current model-based STM tests.

While doing so, I realized I could remove some of the duplicated code by pulling out special cases of insert and remove functions, and modeling things with generic insert/remove abstractions instead.

While writing this I discovered

  • that Sys.rename is not documented to work on anything but files
  • the Windows behaviour of Sys.rename differs in two ways:
    • On Windows it is OK to Sys.rename existing dir to existing file
    • On Windows it is not OK to Sys.rename existing dir to existing dir ("permission denied")

The these reasons the CI tests currently fail.

I've opened a PR and an issue on the compiler for the above.

jmid avatar Mar 06 '23 09:03 jmid

Rebased on main to trigger a fresh run now that https://github.com/ocaml/ocaml/pull/12184 has been merged. As a result, I expect (read: hope) that the trunk tests will now pass... :crossed_fingers:

jmid avatar May 08 '23 15:05 jmid

I've spent some time on this PR today:

  • I refactored the code into a Model module with relevant operations
  • I restructured the error cases in postcond to be more uniform reusing ideas from @shym's Mkfile case
  • I added conditions for the various Windows corner cases that differ
  • Finally I removed 3 spurious "Permission denied" matches that weren't used!

I've observed that the Linux parallel test may now sometimes fail within 200 attempts. I'll wait and see if it needs to be turned back into a negative test again. I've checked manually that the resulting test runs on 5.0 under Linux, MingW, and macOS.

Some of the error cases could still benefit from being rewritten with suitable function abstractions. The overhaul nevertheless represents a good footing for adding symlinks and permissions... :nerd_face:

jmid avatar May 24 '23 15:05 jmid

Rebased on main as I wanted to get an idea of whether this helps on #359. I expect the Windows runs to still fail.

Also (as a note to my self) the 'focused runners' are GitHub actions only - so this will trigger a full testsuite run of Multicoretests-CI despite be5d514.

jmid avatar Jun 07 '23 10:06 jmid

Rebased yesterday to trigger these on the patched 5.1.0~beta1 release. There is clearly some way to go :grimacing: While deciphering errors, I've identified that this has a side-effect of removing an existing empty target dir during a failing Sys.rename:

# Sys.mkdir "sandbox\\eee" 0o755;;
- : unit = ()
# Sys.rename "sandbox" "sandbox\\eee";;
Exception: Sys_error "Invalid argument".
# Sys.file_exists "sandbox\\eee";;
- : bool = false

jmid avatar Jul 14 '23 11:07 jmid

I did a fresh rebase of this PR. As part of it, I removed the pattern-matching on specific error messages, as I found this was digressing into reverse-engineering of Linux/macOS/Windows-specific error message conditions, rather than focusing on a specification. After all the Sys specification says

Every function in this module raises Sys_error with an informative message when the underlying system call signal an error.

The above is considered a clean-up, paving the way for further extensions. It does not solve the Windows issue mentioned above, so the CI should still fail on Windows...

jmid avatar Apr 19 '24 13:04 jmid

I pushed a clean-up commit d2a1345 which

  • accepts the Windows rename regression
  • quantifies all "accept workarounds" with OCaml versions

CI summary

  • 32bit trunk found an unexpected counterexample in STM Sys test parallel in 1/10 runs
  • linux-s390x-5.2 failed while setting up with gnutls_handshake() failed: The TLS connection was non-properly terminated
  • freebsd-amd64-5.2 failed with Internal error

Out of these, 1 is a false alarm and 2 are CI infrastructure errors.

jmid avatar Apr 22 '24 13:04 jmid

Rebased on main with conflicts resolved

jmid avatar May 17 '24 16:05 jmid

CI summary for f8dc3b9 - this is still running focused Sys STM tests:

  • 32bit 5.2 - 0/10 attempts triggered a parallel counterexample
  • 32bit trunk - 4/10 attempts triggered a parallel counterexample
  • Bytecode 5.2 - 3/10 attempts triggered a parallel counterexample
  • Bytecode trunk - 2/10 attempts triggered a parallel counterexample
  • FP 5.2 - 1/10 attempts triggered a parallel counterexample
  • FP trunk - 5/10 attempts triggered a parallel counterexample
  • Linux 5.2 - 4/10 attempts triggered a parallel counterexample
  • Linux 5.2 debug - 2/10 attempts triggered a parallel counterexample
  • Linux trunk - 3/10 attempts triggered a parallel counterexample
  • Linux trunk debug - 2/10 attempts triggered a parallel counterexample - and one assertion failure #447
  • MSVC bytecode trunk - 10/10 sequential tests failed as the Sys.rename fix has been merged to trunk
  • MSVC trunk - 10/10 sequential tests failed as the Sys.rename fix has been merged to trunk
  • MinGW bytecode trunk - 10/10 sequential tests failed as the Sys.rename fix has been merged to trunk
  • MinGW trunk - 10/10 sequential tests failed as the Sys.rename fix has been merged to trunk

It is clearly harder to trigger counterexamples under Linux. For the latter 4 failures, the model should be adjusted to account for the patch merge.

jmid avatar May 21 '24 07:05 jmid