Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork137
Simplify string comparisons in tests and fix bug in String::compareTo#137
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This adds a newline at the end of the file, which is helpful for gitdiff display.
This ensures that checks like: REQUIRE(some_str == "ABC");Actually print the value of some_str instead of just `1` (which is theString object converted to StringIfHelperType).This is implemented by adding a specialization of the Catch StringMakertemplate, so this only takes effect when StringPrinter.h is included.This commit includes it in all the String testcases (even ones that donot strictly use it now) for good measure.
| { | ||
| arduino::Stringstr1("Hello"),str2("Hello"); | ||
| REQUIRE(str1.equals(str2) ==1); | ||
| CHECK(str1.equals(str2) ==1); |
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.
Good morning@matthijskooijman 👋 ☕ Sorry for missing this additional PR to#97. I'd outright approve and merge it but for this sequencedCHECK 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I merged these because they essentially test the same thing. More specifically, they test thatequals,== 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 preferREQUIRE overCHECK. If you could please change that I'll be happy to merge this PR.
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.
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 withNULL 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.
See7deb046 (note that this used to be an addedCHECK line in another commit, but because it really was off-topic for that commit and now adds 2 testcases instead of just oneCHECK line, I moved this into a commit of its own).
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.
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 ofa675d5a (which is merged).
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.
Sorry about that 😊
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.
@matthijskooijman Could you maybe open a separate PR cleaning up the test code?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 woulderronously return 0 (equal) because of a typo.This bug also masked three incorrect checks in related testcases. In twocases, a String was made invalid and then checked to still contain avalue (these were changed to check that the string is invalid) and inone case the wrong string was checked.
This replaces assertions that previously checked the return value ofstrcmp or compareTo, giving the testing framework a bit more informationon the actual comparison happening, so it can show the actual stringsit compares (instead of just the strcmp or compareTo return value).This changes errors like: REQUIRE( strcmp(str.c_str(), "ABC") == 0 ) with expansion: 220 == 0into: 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.cppNote that test_compareTo.cpp was excluded, since that actually needs totest compareTo.Additionally, two more lines were changed manually (one where theArduino string and cstr were reversed, one where compareTo needed toreturn 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 usein other tests.
b3119a9 toc64e2c4Comparecodecov-io commentedJan 25, 2021
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.
|
aentinger left a comment
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.
Thank you@matthijskooijman 🚀
Uh oh!
There was an error while loading.Please reload this page.
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.