8000 [cram] Add locks to the Cram stanza by voodoos · Pull Request #4397 · ocaml/dune · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[cram] Add locks to the Cram stanza #4397

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 7, 2021
Merged

[cram] Add locks to the Cram stanza #4397

merged 4 commits into from
Apr 7, 2021

Conversation

voodoos
Copy link
Collaborator
@voodoos voodoos commented Mar 25, 2021 •
edited
Loading

This PR adds locks to the Cram stanza. This is necessary to run some mutually exclusive tests.
For example we have some tests of the Merlin server that fail randomly when executed concurrently.

Of course the hard part is testing these changes:

  • I had to turn off INSIDE_DUNE but I believe this should not be too dangerous if I then always specify the --root of the workspace on the command line ?
  • I make the assumption that two identical tests that run concurrently should finish at the same second. This is wrong (but luckily pleased the CI), and I guess we should loop and compute a mean.

rgrinberg
rgrinberg approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member
@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me. If you think this should be introduced in 2.9, you should add the appropriate milestone and update CHANGES

@ghost
Copy link
ghost commented Mar 25, 2021

One remark: locks are filenames and a cram stanza apply to a whole subtree. So it's important that the lock names are interpreted relative to where the cram stanza is written rather than where the .t file happens to be.

Might be worth adding a test with a single cram stanza and cram tests at different depth in the tree, to verify that they all take the same lock.

@voodoos voodoos added this to the 2.9 milestone Mar 29, 2021
@voodoos
Copy link
Collaborator Author
voodoos commented Mar 30, 2021

Might be worth adding a test with a single cram stanza and cram tests at different depth in the tree, to verify that they all take the same lock.

Well spotted: if I move one of the two tests that should share the same lock in a subdirectory they don(t run sequentially.

ghost
ghost reviewed Mar 30, 2021
View reviewed changes
@voodoos voodoos force-pushed the cram-locks branch 2 times, most recently from 9349675 to 9a3742a Compare April 1, 2021 16:12
@voodoos
Copy link
Collaborator Author
voodoos commented Apr 1, 2021 •
edited
Loading

@rgrinberg I changed the implementation to have locks always relative to the cram stanza , it should be worth it to review it again.

@voodoos voodoos requested a review from rgrinberg April 2, 2021 22:22
rgrinberg
rgrinberg reviewed Apr 3, 2021
View reviewed changes
rgrinberg
rgrinberg reviewed Apr 3, 2021
View reviewed changes
voodoos added 4 commits April 7, 2021 16:35
@voodoos
Add locks to Cram stanza
8e80e8b 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Add timing-based tests for the locks
671b6d2 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Reference the new option in the docs
f7b7a41 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Add changelog entry
a09c234 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos voodoos merged commit 6993df6 into ocaml:main Apr 7, 2021
voodoos added a commit to voodoos/dune that referenced this pull request Apr 12, 2021
@voodoos
[cram] Add locks to the Cram stanza (ocaml#4397)
2faf22e 
* Add locks to Cram stanza
* Add timing-based tests for the locks
* Reference the new option in the docs
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
voodoos added a commit to voodoos/dune that referenced this pull request Apr 12, 2021
@voodoos
[cram] Add locks to the Cram stanza (ocaml#4397)
9d81999 
* Add locks to Cram stanza
* Add timing-based tests for the locks
* Reference the new option in the docs
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
voodoos added a commit to voodoos/dune that referenced this pull request Apr 12, 2021
@voodoos
[cram] Add locks to the Cram stanza (ocaml#4397)
396d727 
* Add locks to Cram stanza
* Add timing-based tests for the locks
* Reference the new option in the docs
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
voodoos added a commit to voodoos/dune that referenced this pull request Apr 14, 2021
@voodoos
[cram] Add locks to the Cram stanza (ocaml#4397)
1cc6f2f 
* Add locks to Cram stanza
* Add timing-based tests for the locks
* Reference the new option in the docs
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
voodoos added a commit to voodoos/dune that referenced this pull request Jun 4, 2021
@voodoos
[cram] Add locks to the Cram stanza (ocaml#4397)
7b88894 
* Add locks to Cram stanza
* Add timing-based tests for the locks
* Reference the new option in the docs
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
< 7E63 div class="TimelineItem-body" id="ref-commit-7fbfce7"> ejgallego added a commit that referenced this pull request Jun 7, 2021
@voodoos @ejgallego
[cram] Add locks to the Cram stanza (#4397) (#4480)
7fbfce7 
* Add locks to Cram stanza
* Add timing-based tests for the locks
* Reference the new option in the docs
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Co-authored-by: Emilio Jesús Gallego Arias <e+git@x80.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.9
Development

Successfully merging this pull request may close these issues.
2 participants
@voodoos @rgrinberg
0