> You need to get rid of the bare blocks.
done in v4
> This verifies that dshash_dump() doesn't crash, but not that it
> produces useful output. Whether that's worth the infrastructure, I
> don't know.
I don't think dshash_dump() will be deterministic, as keys may hash
to different buckets on different platforms, perhaps. Which is why
I did not think it was a good idea to compare the output from the logs.
Testing that it does not crash seemed worthwhile to increase
the coverage.
> 408). The omissions are: some OOM cases, dshash_delete_bucket()
> successfully deletes something, concurrency case in resizing, most of
> delete_key_from_bucket(), delete_item_from_bucket() iterating more
Yes, we can probably address some of these things with more complexity.
> than once or returning false. Combining your patch with the test suite
> previously mentioned, we get up to 97.1% coverage by lines (396 of
> 408).
This is solid coverage
> So the patch definitely has some value: it adds coverage of 60+ lines
> of code. On the other hand, it still feels to me like we'd be far more
> likely to notice new dshash bugs based on its existing usages than
> because of this. If I were modifying dshash, I wouldn't run this test
> suite first; I'd run the regression tests first.
Sure, but once a bug is fixed, having direct test coverage fo this bug
in a dedicated test suite is a good thing.
The NO_OOM flag that started this discussion would not fit neatly in other
test suites, AFAICT.
--
Sami
