Move big-int unit test to unit/ folder and make it a CATCH-style test [blocks: #2452] #3409
Conversation
tautschnig
commented
Nov 14, 2018
tautschnig
commented
- Each commit message has a non-empty body, explaining why the change was made.
- Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
- Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
- n/a My commit message includes data points confirming performance improvements (if claimed).
- My PR is restricted to a single feature or bugfix.
- n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.
tautschnig
requested review from
chrisr-diffblue,
kroening,
peterschrammel and
smowton
as code owners
unit/big-int/big-int.cpp
Outdated
| return x.scan(input.c_str(), base) == input.c_str() + input.size(); | ||
| } | ||
|
|
||
| SCENARIO("arbitrary precision integers", "[core][big-int][bigint]") |
There was a problem hiding this comment.
Given these aren't really given/when/then style tests you might want to just use the TEST_CASE header (see https://github.com/catchorg/Catch2/blob/master/docs/tutorial.md#test-cases-and-sections)
There was a problem hiding this comment.
Thanks a lot for the guidance, done as suggested.
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: e7b6291).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/91342365
tautschnig
force-pushed
the
big-int-unit-test
branch
from
e7b6291 to
17d96d7
Compare
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 17d96d7).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/91358749
|
@tautschnig, any reason for not merging this? |
|
Missing approval from @kroening ? |
|
This is problematic -- note that the big-int code has a different license. While the tests don't end up in the executable, I am still concerned. |
I don't completely follow: the big-int code always ends up in the binary, so the test wouldn't affect this in any way. |
|
As of now, our approach to documenting which code has which license is much "per directory" -- moving the big-int test code to the unit tests means that the code in there has different licenses. That can be done, but needs clear documenting. |
I'm all for proper documentation, we've had this question come up a few times. Any proposals for how you'd like to have it documented? AFAIK we don't actually have an explicit documentation at the moment and is-in-a-separate-directory is already the case as the moved code lives in |
|
Yes, I also like that particular machine-readable format! |
