Use constants in bytes.rs test code #247
Conversation
src/bytes.rs
Outdated
| for &cap in &[1024, 1025, 1100, 2000, 2047] { | ||
| assert_eq!(1, original_capacity_to_repr(cap)); | ||
| } | ||
| for i in 0..32 { |
There was a problem hiding this comment.
Thanks for the PR. I see where are coming from.
If I am reading it right, this change seems to restrict the numbers being tested to powers of two and powers of 2 + 1. Before, there were more types of numbers being tested.
What are your thoughts?
There was a problem hiding this comment.
You read it right.
I think there is no right answer on deciding test coverage of the function.
Would it be enough if I add 0, 1000, 1100, 2000, 2047?
Or we can consider testing some random-generated numbers.
There was a problem hiding this comment.
You are right... there is no right answer :) There also is the risk of "reimplementing" the original algorithm and introducing bugs in the test.
If you want to pursue the PR, I would merge it w/ the added numbers you listed.
There was a problem hiding this comment.
Please check the changed test code!
I tried to use MIN_ORIGINAL_CAPACITY_WIDTH for the added numbers, too.
|
Got a failing test! |
|
Oh, it's my first cross-compiling issue 😂 |
I think using constants in test code is better for maintenance.