search

mirage / alcotest

A lightweight and colourful test framework
ISC License
456 stars 80 forks source link

Do not use symlink when writing and reading log files. #353

Closed hhugo closed 2 years ago

hhugo commented 2 years ago

Fix #272

hhugo commented 2 years ago

Regression introduced in https://github.com/mirage/alcotest/commit/44e1f1b929e7858a4c5a7a3b2b81200c1ae58a1f

hhugo commented 2 years ago

gentle ping

TheLortex commented 2 years ago

Hi @hhugo, the PR looks sensible. Because I'm not familiar with alcotest I writing out how I think this fixes https://github.com/mirage/alcotest/issues/272

If I understand correctly, the race condition is that when two test suites with the same name are run concurrently, we get the following folders:

When writing to or reading from the log files, the symlink path was used, but as it is updated one of the test suites run will point to the wrong path.

The PR fixes it by using the concrete path (using the uuid) instead for read/write operations.

hhugo commented 2 years ago

I've added a changelog entry and formatted the code.

hhugo commented 2 years ago

Thanks for merging. When should I expect a release with this change ?

TheLortex commented 2 years ago

The plan is to get a release soon enough, I'd like to see some of the maintenance-related PRs merged first, in particular:

@CraigFe: I'm willing to take care of this if that's okay for you !

hhugo commented 2 years ago

https://github.com/mirage/alcotest/pull/348 would also be nice. It will likely break packages on opam and will require adding constraints.