dune
dune copied to clipboard
ocaml
enable exec + watch tests with timeout
The exec-watch tests have been disabled in https://github.com/ocaml/dune/pull/7138 due to flakiness. This is a bit unfortunate, as these tests are useful for preventing regressions.
What I suggest in this PR is to add a timeout to the wait-for-file.sh script. So after e.g. 2 seconds the script will exit with an error if the file does not exist. This should prevent a test from hanging forever.
See for example b705448 (#10384), which is the result I got when I ran the tests on my MacOS machine. It helped me find the reason why the test was hanging forever (sed + symlink issue) and allowed me to fix it
Are you asking whether the exec + watch tests are still flaky and would benefit from a timeout?
I haven't looked at the PR closely and don't know the situation of the tests so I don't know if this PR is useful. I was suggesting that you take over reviewing/merging this PR.
Thanks for the patch @tatchi. Apologies for the delay in getting to this. I agree that it's better to enable this test and accommodate for its flakiness than disabling it, especially since as you've addressed in this PR, the test has bitrotted due to not being run.
I'm doing a small cleanup of the commit history of this PR.
The one thing I want to figure out before merging this is what the behavior should be when a timeout occurs. Currently the wait-for-file.sh script prints a message and exits 1 which will cause tests to fail. Since the failure mode that leads to a timeout is non-deterministic, this would cause tests to non-deterministically fail, which is something I'd like to avoid.
The alternative I see is to silently exit on timeouts, but the risk here of course is that it leads us to miss regressions in exec -w (though it would still be an improvement over disabling the test outright). If we made the timeout silent, would the test have still helped you catch the bug you mentioned, or would it only have been useful if timeouts caused the test to fail?