Add more tests

[Stupremee]

This PR adds several tests to increase the test coverage and more benchmarks.

---

This change is 

[codecov]

Codecov Report

Merging #61 into master will decrease coverage by 0.77%.
The diff coverage is n/a.

Impacted Files	Coverage Δ
src/map.rs	87.16% <0%> (-1.82%)	⬇️
src/raw/mod.rs	86.73% <0%> (-1.03%)	⬇️
src/map_ref.rs	88.88% <0%> (+9.25%)	⬆️

[jonhoo]

(CI failure is due to miri missing on 2020-02-21 nightly: https://rust-lang.github.io/rustup-components-history/)

[Stupremee]

Should I fix the ci issue?

[jonhoo]

Should I fix the ci issue?

No, just leave it -- it'll likely be fixed tomorrow-ish.

[Stupremee]

Are there some tests / benchmarks that you would like to have in Flurry?
Because I wasn't able to find any missing tests from the Jdk.
I will just add some benchmarks and then would be ready to finish the PR.

[jonhoo]

The tests that I know of are the JDK tests, the JSR166 tests, and the hashbrown tests. Ideally we'd have pretty much all the tests from all those three.

[Stupremee]

I think all tests are implemented that are compatible with flurry.
I think I will leave the benchmarks as they are.

[domenicquirl]

Thanks for working on this! A few comments from looking through the diff:

• You added the OneBucketHasher from basic.rs to basic_ref.rs to use it there as well, and there is also ZeroHasher in hasher.rs. All of these hash everything to 0. It would probably make sense to have this only once and shared it everywhere it is needed.
• It looks like you simplified uses of map.pin() in most tests. @jonhoo did you look at this in your previous review?
  • For some methods, such as insert_and_remove, this is probably okay since they also only used one pin() before.
  • Other methods use several pins, and we have to be careful to not involuntarily exclude testing that using multiple HashMapRefs works as intended. For example in update, we want the updates to be visible if we use a different Ref later. I think we should keep testing that.
  • The XYZ_equal methods previously tested HashMapRef equality against HashMapRef itself and against HashMap. Since these equality traits are added together with HashMapRef, I think the test should continue to check this.
• Concurrent tests:
  • When setting up a map at the start of a concurrent test, e.g. here, if we use a guard and leave it around afterwards, then this guard will pin an epoch for the entire duration of the test. To avoid this, the guard can be put inside of a local scope, like here:
    https://github.com/jonhoo/flurry/blob/master/tests/basic.rs#L253-L262
  • Also note that this uses map.guard() instead of epoch::pin(), which we could change while we're at it.
  • This one might be me not exactly keeping up with everything that happend here (if so, sorry 😅 ), but I didn't see new versions of some of the removed concurrency tests, for example concurrent_remove. If there isn't a new implementation, I would just keep the old tests so we don't lose anything.
• Lastly, there is a typo here, but that'll be quick to change 😊

[domenicquirl]

Wonderful! Don't know why one of the CI targets failed, but all of my comments have been addressed with the new commits and this seems to be unrelated to the code, so I'm happy to approve this now.

[jonhoo]

The miri failure is because miri was broken on the last nightly. I think the newest one has it again though, so I'll re-run.

[jonhoo]

This looks great! I only have one remaining request I think: it'd be good if the new test files you've added that are ports of tests from elsewhere have that reflected in the name. If they are from the JDK test suite, then add them under the jdk directory. If they are from the JSR166 test suite, add them under a jsr166 folder.

[jonhoo]

Reviewed 1 of 2 files at r1, 1 of 5 files at r3, 5 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[jonhoo]

🎉 Thank you!