Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds tests for get_one #56

Merged
merged 9 commits into from
Jun 30, 2020
Merged

Adds tests for get_one #56

merged 9 commits into from
Jun 30, 2020

Conversation

mlodato517
Copy link
Contributor

@mlodato517 mlodato517 commented Jun 27, 2020

This PR
Attempting to help along with #31.

Notes
It appeared that get_one wasn't tested. I'm not sure if these tests are in the right place or if they should live in read/mod. Or maybe this isn't tested because it's not valuable functionality? 🤷

I tried to handle both inner cases - ValuesInner::Short and ValuesInner::Long. I'm not 100% sure it's necessary to test ValuesInner::Long because, although it's supported, it's clearly documented that, in the case of multiple values, the exact return value is not guaranteed. So it's nice to make sure it doesn't explode with multiple values but it also doesn't feel particularly useful asserting any particular returned value. I also feel pretty badly about the magic number with no hard-reference to BAG_THRESHOLD. If we do want to improve the readability there, it looked like there was a with_capacity on Options but I didn't see one for evmap so I wasn't sure how to do that idiomatically. I probably read the docs wrong. It looks like you can do something like:

Options::default().with_capacity(BAG_THRESHOLD).construct()

although maybe there's a slightly more compact method.

Finally, I don't know how to check code coverage before merging this so acknowledging that there's no guarantee the coverage will increase, I'm fine with just chucking this PR (but would still like to help if any more specific guidance is available!). Looks like it shows up right there in the PR! Neat!


This change is Reviewable

Attempts to test both types of inner values - ValuesInner::Short and
ValuesInner::Long.
@codecov
Copy link

codecov bot commented Jun 27, 2020

Codecov Report

Merging #56 into master will increase coverage by 1.68%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   79.13%   80.82%   +1.68%     
==========================================
  Files          11       11              
  Lines         997     1043      +46     
==========================================
+ Hits          789      843      +54     
+ Misses        208      200       -8     
Impacted Files Coverage Δ
src/values.rs 89.26% <100.00%> (+8.90%) ⬆️
tests/lib.rs 98.00% <100.00%> (+0.04%) ⬆️
src/read/mod.rs 72.41% <0.00%> (+2.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b72919f...c907045. Read the comment docs.

tests/lib.rs Outdated Show resolved Hide resolved
tests/lib.rs Outdated Show resolved Hide resolved
tests/lib.rs Outdated Show resolved Hide resolved
@mlodato517 mlodato517 marked this pull request as ready for review June 27, 2020 03:23
@jonhoo
Copy link
Owner

jonhoo commented Jun 27, 2020

This is great, thank you! I left some comments :)

Mark Lodato added 3 commits June 27, 2020 12:58
tests/lib.rs Outdated Show resolved Hide resolved
src/values.rs Outdated Show resolved Hide resolved
src/values.rs Show resolved Hide resolved
src/values.rs Show resolved Hide resolved
@mlodato517
Copy link
Contributor Author

Also, slightly unrelated - I see the CI failing scarily (to someone who didn't know why). It seems like the version of evmap in benchmark/Cargo.toml can be updated to the current 11.0.0-alpha.1. Is that something to fix or is that purposefully not being upgraded until a non-alpha/beta/whatever release lands?

@jonhoo
Copy link
Owner

jonhoo commented Jun 30, 2020

Oh, yeah, good catch! If you want to include a quick fix for that in this PR too, that'd be great.

@mlodato517
Copy link
Contributor Author

mlodato517 commented Jun 30, 2020

Oh, yeah, good catch! If you want to include a quick fix for that in this PR too, that'd be great.

Sorry, just saw this. Do you mind if I make it a new PR? I already had a new PR ready to go with the changes and figured it'd keep each PR's focus clean? But I'm also 100% down to combine them to reduce overhead!

I could squash all the existing commits and then add the new commit that bumps the dependency version and not squash those two if you'd prefer 👍

Edit:
That PR is here (but I'm still open to closing it and merging the two!).

@jonhoo jonhoo merged commit 75c81ef into jonhoo:master Jun 30, 2020
@jonhoo
Copy link
Owner

jonhoo commented Jun 30, 2020

Beautiful! I merged that PR, and squashed and merged this one. Thanks for all your work on this :D

@mlodato517 mlodato517 deleted the increase-test-coverage-get-one branch June 30, 2020 19:51
@mlodato517
Copy link
Contributor Author

It's been my pleasure! Are there any other tests I should focus on next? Or any other things you've been wanting to get to but haven't had time? I really want to start contributing and learning/writing rust. Seeing as I've already learned a handful of things from this, I'd say continuing on would be fruitful. So I'm ready and rearing for anything you need or know about someone else needing! 👍

@jonhoo
Copy link
Owner

jonhoo commented Jun 30, 2020

I think an awesome thing would be to adopt parts of the test suites from hashbrown and indexmap. They have extensive test suites, which evmap could possibly benefit a lot from. You'll have to work around evmap having a slightly different API, but it shouldn't be too bad, at least for the simpler tests. Some quicktests would also be fantastic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants