> 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
v4-0001-Add-test-module-for-dshash.patch
Description: Binary data
