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

loyd · 2023-10-02T18:14:31Z

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).

Add fuzzing tests for public API
Fix Slab::unique_iter panics when the slab is empty #73 (unique_iter)
Fix panics in get() for some keys
Satisfy clippy

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 · 2023-10-02T18:23:08Z

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 · 2023-10-02T23:03:59Z

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!

tests/fuzzing.rs

src/iter.rs

loyd · 2023-10-03T07:55:27Z

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.

Fixes several clippy's lints: * non_canonical_clone_impl * needless_borrow

Prevents public methods from panicking if received keys that haven't been produced by the slab.

Fixes panics in `Slab::unique_iter()` in case of an empty slab. Fixes hawkw#73

loyd · 2023-10-03T19:51:23Z

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)?

hawkw

this looks great, thank you!

i guess we'll have to change the MSRV CI job to be build only, rather than running the tests, because of the memory_stats thing. so, i'm fine with an indexmap v2 dep if you prefer.

Adds property-based tests that check the following properties: * API never panics. * Active entries cannot be overridden until removed. * The slab doesn't produce overlapping keys. * The slab doesn't leave "lost" keys. * `get()`, `get_owned`, and `contains()` are consistent. * `RESERVED_BITS` are actually not used.

* Add `msrv` job * Add `RUST_BACKTRACE=1` * Add `-Dwarnings` * Fix clippy warnings in benches * Make job naming more consistent

loyd · 2023-10-04T07:21:16Z

@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 · 2023-10-04T18:09:19Z

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!

Fixes several clippy's lints: * non_canonical_clone_impl * needless_borrow

Prevents public methods from panicking if received keys that haven't been produced by the slab.

Fixes panics in `Slab::unique_iter()` in case of an empty slab. Fixes #73

Adds property-based tests that check the following properties: * API never panics. * Active entries cannot be overridden until removed. * The slab doesn't produce overlapping keys. * The slab doesn't leave "lost" keys. * `get()`, `get_owned`, and `contains()` are consistent. * `RESERVED_BITS` are actually not used.

* Add `msrv` job * Add `RUST_BACKTRACE=1` * Add `-Dwarnings` * Fix clippy warnings in benches * Make job naming more consistent

### v0.1.7 (2023-10-04) #### Bug Fixes * index out of bounds in `get()` and `get_owned()` (#88) (fdbc930) * **unique_iter:** prevent panics if a slab is empty (#88) (bd599e0, closes #73)

hawkw · 2023-10-04T19:39:50Z

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

loyd changed the title ~~Fix bugs in get() and co + fuzzing tests~~ Fix bugs in get() and co + property based tests Oct 2, 2023

hawkw reviewed Oct 2, 2023

View reviewed changes

tests/fuzzing.rs Outdated Show resolved Hide resolved

src/iter.rs Outdated Show resolved Hide resolved

loyd force-pushed the main branch from 79aab58 to b933e3e Compare October 3, 2023 19:38

loyd added 3 commits October 3, 2023 23:49

style: satisfy clippy

623c0c9

Fixes several clippy's lints: * non_canonical_clone_impl * needless_borrow

fix: index out of bounds in get() and get_owned()

3dc7c13

Prevents public methods from panicking if received keys that haven't been produced by the slab.

fix(unique_iter): prevent panics if a slab is empty

f5749ce

Fixes panics in `Slab::unique_iter()` in case of an empty slab. Fixes hawkw#73

loyd force-pushed the main branch from b933e3e to d609a39 Compare October 3, 2023 19:49

loyd force-pushed the main branch 2 times, most recently from b1d9126 to 26111f8 Compare October 3, 2023 19:59

hawkw approved these changes Oct 3, 2023

View reviewed changes

loyd force-pushed the main branch 5 times, most recently from c4701db to 7a70384 Compare October 4, 2023 07:08

fix(ci): move msrv check to a dedicated job

14f5826

* Add `msrv` job * Add `RUST_BACKTRACE=1` * Add `-Dwarnings` * Fix clippy warnings in benches * Make job naming more consistent

loyd force-pushed the main branch from 7a70384 to 14f5826 Compare October 4, 2023 07:16

hawkw merged commit 5495def into hawkw:main Oct 4, 2023
8 checks passed

hawkw pushed a commit that referenced this pull request Oct 4, 2023

style: satisfy clippy (#88)

5666378

Fixes several clippy's lints: * non_canonical_clone_impl * needless_borrow

hawkw pushed a commit that referenced this pull request Oct 4, 2023

fix: index out of bounds in get() and get_owned() (#88)

fdbc930

Prevents public methods from panicking if received keys that haven't been produced by the slab.

hawkw pushed a commit that referenced this pull request Oct 4, 2023

fix(unique_iter): prevent panics if a slab is empty (#88)

bd599e0

Fixes panics in `Slab::unique_iter()` in case of an empty slab. Fixes #73

hawkw pushed a commit that referenced this pull request Oct 4, 2023

chore(ci): move msrv check to a dedicated job (#88)

eccd94c

* Add `msrv` job * Add `RUST_BACKTRACE=1` * Add `-Dwarnings` * Fix clippy warnings in benches * Make job naming more consistent

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

loyd commented Oct 2, 2023 •

edited

Loading

loyd commented Oct 2, 2023

hawkw commented Oct 2, 2023

loyd commented Oct 3, 2023 •

edited

Loading

loyd commented Oct 3, 2023 •

edited

Loading

hawkw left a comment

loyd commented Oct 4, 2023 •

edited

Loading

hawkw commented Oct 4, 2023

hawkw commented Oct 4, 2023 •

edited

Loading

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

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

Conversation

loyd commented Oct 2, 2023 • edited Loading

loyd commented Oct 2, 2023

hawkw commented Oct 2, 2023

loyd commented Oct 3, 2023 • edited Loading

loyd commented Oct 3, 2023 • edited Loading

hawkw left a comment

Choose a reason for hiding this comment

loyd commented Oct 4, 2023 • edited Loading

hawkw commented Oct 4, 2023

hawkw commented Oct 4, 2023 • edited Loading

loyd commented Oct 2, 2023 •

edited

Loading

loyd commented Oct 3, 2023 •

edited

Loading

loyd commented Oct 3, 2023 •

edited

Loading

loyd commented Oct 4, 2023 •

edited

Loading

hawkw commented Oct 4, 2023 •

edited

Loading