Simplify string comparisons in tests and fix bug in String::compareTo #137
Conversation
This adds a newline at the end of the file, which is helpful for git diff display.
This ensures that checks like:
REQUIRE(some_str == "ABC");
Actually print the value of some_str instead of just `1` (which is the
String object converted to StringIfHelperType).
This is implemented by adding a specialization of the Catch StringMaker
template, so this only takes effect when StringPrinter.h is included.
This commit includes it in all the String testcases (even ones that do
not strictly use it now) for good measure.
| /************************************************************************************** | ||
| * TEST CODE | ||
| **************************************************************************************/ | ||
|
|
||
| TEST_CASE ("Testing String::equals(const String &) with exit status PASS", "[String-equals-01]") | ||
| { | ||
| arduino::String str1("Hello"), str2("Hello"); | ||
| REQUIRE(str1.equals(str2) == 1); | ||
| CHECK(str1.equals(str2) == 1); |
There was a problem hiding this comment.
Good morning @matthijskooijman 👋 ☕ Sorry for missing this additional PR to #97. I'd outright approve and merge it but for this sequenced CHECK constructs.
- Why are using
CHECKinstead ofREQUIRE? - Why are there 3 checks within one test case? As I've written in my feedback on Strings without null-termination #97 the usual testing idiom is: 1 Testcase - 1 Assertion. If you have multiple assertions within one testcase you are testing multiple things which is mudding the water. Please split these tests up.
There was a problem hiding this comment.
I merged these because they essentially test the same thing. More specifically, they test that equals, == and != (which are all just different ways to call the same code) behave correctly in the same situation. I could split them up, but that would essentially just mean more duplication of code for creating the test situation (which admittedly isn't much code, but still).
So I'd still think these are better as three checks in a single testcase, but if you insist, I'm also fine with splitting them up.
There was a problem hiding this comment.
I see 👀 I've not yet made up my mind on how to best approach these situations where one function is basically just a wrapper and directly calls another function. My main intention behind objecting to your test-code-merging was on the basis that I don't want to set a precedent for bad test design. Looking deeper into the issue I do see the reasons for you doing so. Nonetheless I'd very much prefer REQUIRE over CHECK. If you could please change that I'll be happy to merge this PR.
There was a problem hiding this comment.
Ok, I changed the PR as you asked.
Note that in addition to one function that wraps another, a second use of CHECK was where you test one situation, and want to check multiple postconditions of this situation. In this case, assigning a string with NULL and checking that the string is not still the same as before and is marked as invalid. I've now duplicated this testcase, which leads to a little bit more duplicate code than the other case. Also, this means that the testcase description now not only needs to indicate what situation is being tested, but also what postcondition is being checked.
See 7deb046 (note that this used to be an added CHECK line in another commit, but because it really was off-topic for that commit and now adds 2 testcases instead of just one CHECK line, I moved this into a commit of its own).
There was a problem hiding this comment.
Heh, seems you merged directly after I pushed, while I was making one more tweak to my commits (making the history a little cleaner and adding one more testcase). Now that it's merged, not really relevant to spend more time on.
For anyone reading my previous comment, note that the commit referenced there is thus not actually merged, but is still part of a675d5a (which is merged).
There was a problem hiding this comment.
@matthijskooijman Could you maybe open a separate PR cleaning up the test code?
There was a problem hiding this comment.
It was mostly a cleanup of git history, which I can't fix anymore (but it wasn't a big cleanup) and one added testcase just for consistency, which isn't really important anyway (not worth setting up another PR for, IMHO), so I would just leave this as it is now.
There was a problem hiding this comment.
Roger that. I'll be sure to wait a bit longer next time 👍
This expands the existing tests for String.equals to also test operator == and !=. These should be equivalent (the operators just call equals), but add tests to ensure this.
When comparing an invalid String object to a non-empty char*, this would erronously return 0 (equal) because of a typo. This bug also masked three incorrect checks in related testcases. In two cases, a String was made invalid and then checked to still contain a value (these were changed to check that the string is invalid) and in one case the wrong string was checked.
This replaces assertions that previously checked the return value of
strcmp or compareTo, giving the testing framework a bit more information
on the actual comparison happening, so it can show the actual strings
it compares (instead of just the strcmp or compareTo return value).
This changes errors like:
REQUIRE( strcmp(str.c_str(), "ABC") == 0 )
with expansion:
220 == 0
into:
REQUIRE( str == "ABC" );
with expansion:
"XYZ" equals: "ABC"
These changes were done using the following commands:
sed -i 's/REQUIRE(strcmp(\([^,]*\).c_str(), \([^,]*\).c_str()) == 0)/REQUIRE(\1 == \2)/g' *
sed -i 's/REQUIRE(strcmp(\([^,]*\).c_str(), \([^,]*\)) == 0)/REQUIRE(\1 == \2)/g' *
sed -i 's/REQUIRE(\([^.]*\).compareTo(\([^)]*\)) == 0)/REQUIRE(\1 == \2)/g' test_String.cpp test_characterAccessFunc.cpp test_operators.cpp test_substring.cpp
Note that test_compareTo.cpp was excluded, since that actually needs to
test compareTo.
Additionally, two more lines were changed manually (one where the
Arduino string and cstr were reversed, one where compareTo needed to
return non-zero).
Also note that this relies on the operator == defined by String itself,
but since that is subject of its own tests, this should be ok to use
in other tests.
matthijskooijman
force-pushed
the
simplify-string-tests
branch
from
b3119a9 to
c64e2c4
Compare
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
=======================================
Coverage 96.41% 96.42%
=======================================
Files 14 14
Lines 837 839 +2
=======================================
+ Hits 807 809 +2
Misses 30 30
Continue to review full report at Codecov.
|
While trying to fix some problems in #97, I saw that test failures involving string comparisons were quite hard to debug, since the actual string values were not printed. This PR changes that, as well as simplifying the REQUIRE lines to use the normal
==operator. To make sure that Arduino String objects are properly printed on failure, this also adds a specialization of the Catch StringMaker template.While making these changes, I also uncovered a small bug in
String.compareTowhich is also fixed.This is the third iteration of these changes. I initially tried using the Catch
Requiresyntax, but that was a bit cumbersome (needs to convert all strings tostd::stringfor comparison). Then I created a custom Matcher for Arduino String object, which made things a bit cleaner, but then I realized that just using==would use the String class' ownoperator ==, which is fine for most testcases.