search

hawkw / sharded-slab

a lock-free concurrent slab (experimental)
MIT License
269 stars 17 forks source link

Fix bugs in get() and co + property based tests #88

Closed loyd closed 12 months ago

loyd commented 12 months ago

I've written some fuzzing property based tests for public API. Fortunately (or not), it's found only simple bugs with boundary checks (caught in staging, the reason why I've decided to check the slab in such a way).

P.S. "fuzzing" is not an entirely valid name here. Initially, It checked only the absence of panics, so naming was more appropriate. Then, more properties were added. Would you happen to have any ideas on how to call it now?

loyd commented 12 months ago

Oops, it seems that the hashbrown crate moved to newer rustc. Should I downgrade the indexmap or allow CI to build tests with newer rustc?

hawkw commented 12 months ago

Thanks for the PR, this is really cool! I have a few minor suggestions, but overall, it's really nice to have this additional testing, so thanks for all your work on this!

In re: your questions:

P.S. "fuzzing" is not an entirely valid name here. Initially, It checked only the absence of panics, so naming was more appropriate. Then, more properties were added. Would you happen to have any ideas on how to call it now?

Honestly, I would probably name it "properties" or something, instead of "fuzzing"...but I'm fine with "fuzzing", too...

Oops, it seems that the hashbrown crate moved to newer rustc. Should I downgrade the indexmap or allow CI to build tests with newer rustc?

It seems pretty unfortunate to bump our MSRV just for a test-only dependency. I'm fine with a dev-dependency on indexmap v1.0, but I'm also okay with changing the CI configuration so that we just build the actual library on MSRV, rather than actually running tests. I wouldn't expect the tested behavior to change based on the Rust version, unless we're deeply unlucky, so just checking that the lib itself compiles for our MSRV is probably fine...

One small, procedural point: we use the Git history to generate this crate's changelog. If you don't mind, it would be really nice if the two bugfixes (the one for UniqueIter and get for random keys) could be pulled out into separate commits from the commit that actually adds the property tests (and ideally, the clippy fix would be a separate commit as well). That way, they will generate separate changelog entries. If you don't have time to do an admittedly kind of annoying rebase, that's fine, but it would be nicer IMO.

Thanks again for working on this!

loyd commented 12 months ago

If you don't have time to do an admittedly kind of annoying rebase, that's fine, but it would be nicer IMO.

No problem, I'll do it. A good changelog is important.

loyd commented 12 months ago

It seems pretty unfortunate to bump our MSRV just for a test-only dependency.

indexmap is downgraded to the version which supports the current MSRV. However, it seems memory-stats violates MSRV already (failing CI in master).

Also, I updated the commits. Are they good enough for the CHANGELOG generation? Text from my previous PR was added to CHANGELOG. Does it still work the same way (should I also edit my first comment)?

loyd commented 12 months ago

@hawkw, I've fixed CI (see the latest commit's message + changes).

Oh, it seems because of renaming the "required checks" logic doesn't work. Should I rename it back? Also, why is clippy not required?

hawkw commented 12 months ago

Oh, it seems because of renaming the "required checks" logic doesn't work. Should I rename it back? Also, why is clippy not required?

I'll just fix those in the GitHub settings. Thanks for taking care of it!

hawkw commented 12 months ago

Okay, I just published v0.1.7, which includes the bugfixes. Thank you so much for working on this!