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

Add initial parking_lot_core tests #191

Merged
merged 3 commits into from
Nov 16, 2019
Merged

Conversation

faern
Copy link
Collaborator

@faern faern commented Nov 16, 2019

This library could use more tests, to be more robust. This was one of the things pointed out in the review of the integration into std in rust-lang/rust#56410

WTF::ParkingLot has a number of tests we can take inspiration from. And this is somewhat of a port of some of those tests. However, when I ported it 1:1 I found a race condition, so I had to implement the semaphore a bit differently.

I also got away without the tests relying on a working mutex or condvar implementation. Because we can't make the parking_lot_core tests depend on the higher level parking_lot, nor do we want to depend on locking primitives in std if we aim to become the base for those implementations one day.

@faern faern requested a review from Amanieu November 16, 2019 14:15
@Amanieu
Copy link
Owner

Amanieu commented Nov 16, 2019

LGTM. Do you think it would be possible to extend these to cover unpark_filter and unpark_requeue? Or would this be better done as a separate set of tests?

@faern
Copy link
Collaborator Author

faern commented Nov 16, 2019

I'm aiming for adding more tests. So we at least touch the entire public API of parking_lot_core at least once. Not sure if those would use the semaphore, probably not(?)
And I felt I should add what WTF::ParkingLot was testing first, and then extend once we have agreed this type of testing is sane.

@Amanieu
Copy link
Owner

Amanieu commented Nov 16, 2019

bors r+

bors bot added a commit that referenced this pull request Nov 16, 2019
191: Add initial parking_lot_core tests r=Amanieu a=faern

This library could use more tests, to be more robust. This was one of the things pointed out in the review of the integration into std in rust-lang/rust#56410

`WTF::ParkingLot` has a number of tests we can take inspiration from. And this is somewhat of a port of some of those tests. However, when I ported it 1:1 I found a race condition, so I had to implement the semaphore a bit differently.

I also got away without the tests relying on a working mutex or condvar implementation. Because we can't make the `parking_lot_core` tests depend on the higher level `parking_lot`, nor do we want to depend on locking primitives in std if we aim to become the base for those implementations one day.

Co-authored-by: Linus Färnstrand <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 16, 2019

@bors bors bot merged commit e716214 into Amanieu:master Nov 16, 2019
@faern faern deleted the add-initial-core-tests branch November 24, 2019 20:28
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.

2 participants