-
Notifications
You must be signed in to change notification settings - Fork 52
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
Productionize testing + add CI #33
Conversation
2801604
to
6c7e46e
Compare
bf514d4
to
707aaad
Compare
ACK 707aaad I didn't review any of the CI stuff more than a glance, and didn't test on 32-bit hosts yet (I assume CI is doing it!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested approach ACK 707aaad with a few minor comments. Each commit builds cleanly. Did not review or test the CI. Tested only on Debian 5.10.28-1 (2021-04-09) x86_64 GNU/Linux.
707aaad
to
e433b94
Compare
Various changes: * Simplify logic to determine capacity of test * Also test for cases where capacity > 2*bits-1
f149995
to
e05ed05
Compare
Smoke test at e05ed05
s/succesfull/successful/ |
* Rename function from TestAll to TestExhaustive * Use C++ wrapper interface * Test all implementations simultaneously, and compare them * No need to report counts back to caller
* Rename from TestRand to TestRandomized * Use C++ wrapper interface * Add a lot more sanity checks * Compare more results between implementations * Test cases where the number of elements exceeds the capacity, but not when cancelling out duplicates.
e05ed05
to
84b87d5
Compare
Ok, this is ready I think. |
ACK 84b87d5 |
ACK 84b87d5 per |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a very cursory review and ran the tests. Looks good.
long long complexity = std::stoll(arg, &len); | ||
if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using complexity as a uint64_t later. Why not:
long long complexity = std::stoll(arg, &len); | |
if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) { | |
auto complexity = static_cast<uint64_t>(std::stoll(arg, &len)); | |
if (complexity >= 1 && len == arg.size() && (complexity <= std::numeric_limits<uint64_t>::max() >> 10)) { |
try { | ||
test_complexity = 0; | ||
long long complexity = std::stoll(arg, &len); | ||
if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something special about std::numeric_limits<uint64_t>::max() >> 10
? Perhaps make a variable named max_complexity
and comment what's special about that value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a test_complexity << 10
; this tests prevents that from overflowing.
if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) { | ||
test_complexity = complexity; | ||
} | ||
} catch (const std::logic_error&) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What logic error is this potentially catching? The only exceptions that stoll()
can throw are std::invalid_argument
and std::out_of_range
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both std::invalid_argument
and std::out_of_range
are subclasses of std::logic_error
, so this way we can avoid 2 catch statement.
void TestComputeFunctions() { | ||
for (uint32_t bits = 0; bits <= 256; ++bits) { | ||
for (uint32_t fpbits = 0; fpbits <= 512; ++fpbits) { | ||
std::vector<size_t> table_max_elements(1025); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::array
?
if (capacity > 0) CHECK(table_max_elements[capacity] == 0 || table_max_elements[capacity] > table_max_elements[capacity - 1]); | ||
} | ||
|
||
std::vector<size_t> table_capacity(513); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::array
?
@jnewbery Feel free to PR your comments here. |
Various improvements to testing:
test-exhaust
totest
Also fix two bugs discovered while writing these improvements: