Skip to content

Concurrent tests#66

Open
icristescu wants to merge 2 commits intomirage:mainfrom
icristescu:concurrent_tests
Open

Concurrent tests#66
icristescu wants to merge 2 commits intomirage:mainfrom
icristescu:concurrent_tests

Conversation

@icristescu
Copy link
Contributor

@icristescu icristescu commented Sep 2, 2019

No description provided.

@craigfe
Copy link
Member

craigfe commented Sep 2, 2019

I think this test should be avoided on Windows using something like this.

Also, we should probably add a Windows CI runner so that such failures are obvious.

samoht
samoht reviewed Sep 2, 2019
View reviewed changes
test/unix/concurrent.ml Outdated
in
let pid = Unix.fork () in
if pid <> 0 then (
Unix.sleepf 0.001;
Copy link
Member

@samoht samoht Sep 2, 2019

Choose a reason for hiding this comment

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

this might be a bit fragile, but I guess that's good enough for now. We can restructure this later if we see that this the tests is not deterministic enough.

Copy link
Member

@craigfe craigfe Feb 25, 2020

Choose a reason for hiding this comment

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

Now that we have a Private functor abstraction, could we add a hook for the creation of the lock file to test this more consistently?

It would be nice to have these tests merged so that they're not a maintenance burden to you 🙂

Copy link
Contributor Author

@icristescu icristescu Feb 27, 2020

Choose a reason for hiding this comment

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

Thanks for the idea, I removed all calls to Unix.sleep.

@craigfe craigfe mentioned this pull request Sep 2, 2019
Open
@icristescu icristescu force-pushed the concurrent_tests branch 6 times, most recently from 2903fa0 to 08a9d79 Compare September 12, 2019 08:21
@icristescu icristescu mentioned this pull request Sep 12, 2019
Closed
@icristescu icristescu force-pushed the concurrent_tests branch 2 times, most recently from 94337ed to 3fcfb43 Compare December 13, 2019 09:43
@icristescu icristescu force-pushed the concurrent_tests branch 2 times, most recently from 41b8c7a to 99936e9 Compare February 27, 2020 10:39
@icristescu icristescu force-pushed the concurrent_tests branch 3 times, most recently from 8649a6f to 99c9a1c Compare June 30, 2020 19:29
@craigfe
Copy link
Member

craigfe commented Jul 13, 2020

I'm looking into the spurious test failures here. Will start with a rebase.

@samoht
Copy link
Member

samoht commented Apr 7, 2021

Keeping this open as this seems to be a real synchronisation issue and need more investigation.

@samoht
Copy link
Member

samoht commented Apr 28, 2021

@craigfe is planning to have a look at it at one point :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samoht samoht samoht left review comments

+1 more reviewer

@craigfe craigfe craigfe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@icristescu @craigfe @samoht