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

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

Merged
merged 3 commits into from
Jun 13, 2022

Conversation

hhugo
Copy link
Contributor

@hhugo hhugo commented Apr 28, 2022

Fix #272

@hhugo
Copy link
Contributor Author

hhugo commented Apr 28, 2022

Regression introduced in 44e1f1b

@hhugo
Copy link
Contributor Author

hhugo commented May 5, 2022

gentle ping

@TheLortex
Copy link
Member

Hi @hhugo, the PR looks sensible. Because I'm not familiar with alcotest I writing out how I think this fixes #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:

  • E0965BF9-... logs for 1st run
  • 6DDB68D5-... logs for 2nd run
  • <suite_name> symlink to either E0965BF9-... or 6DDB68D5-...

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.

Copy link
Member

@TheLortex TheLortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR.

My only complaints are that two CI jobs are failing:

@hhugo
Copy link
Contributor Author

hhugo commented Jun 13, 2022

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

@TheLortex TheLortex merged commit d6ccf18 into mirage:main Jun 13, 2022
@hhugo hhugo deleted the fix-272 branch June 13, 2022 16:12
@hhugo
Copy link
Contributor Author

hhugo commented Jun 13, 2022

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

@TheLortex
Copy link
Member

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
Copy link
Contributor Author

hhugo commented Jun 14, 2022

#348 would also be nice. It will likely break packages on opam and will require adding constraints.

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.

spurious test suite failures in automated builds
2 participants