bxparks
commented
6 years ago One bit of context that I left out in my original post which might help you digest all this. I was trying to create 2 example tests:
- case 1: one which succeeds (intentionally) on AVR but fails on 32-bit environments, and
- case 2: one which succeeds (accidentally) on 32-bit environments, but fails on AVR
I was successful in case 1. I'm pretty sure that I succeeded in case 2, because the if-statement does what I want, but that's when I found that assertLess() doesn't do what the if-statement does.
Case 2 is where mock unit tests running on some other (32-bit) environments will have trouble detecting problems with code that will fail on AVR.
wmacevoy
Summary
The following problems were observed in latest code on
master. Some (maybe all) of these problems were also observed in 2.2.0, but I don't recall if the behavior was exactly the same. The code shown below was tested on an Arduino Nano (ATmega328P), a Teensy LC (ARM Cortex-M0+, 32-bit), and a NodeMCU v1.0 (ESP8266, 32-bit).Problem 1: With the default compiler warning settings, the compiler warnings about integer type mismatch are not printed. The
assertXxx()sometimes produces unexpected results for non-portable code.Expected result: A compile time failure would be preferable, since assert statements with mixed integer types are often non-portable and doesn't do what the user actually intended.
Problem 2:
assertLess()produces an incorrect result on 32-bit processors.Expected result:
assertLess()should behave exactly like its equivalentif-statementcounterpart on a 32-bit processor.Preamble
In the revised templatized code in 2.3.x, the various
assertXxx()macros eventually evaluate to the templatizedassertion()method:which takes an instance of one of the templatized
compareXxx()methods in:The problem occurs when the type of
AandBare mixed signed and unsigned integers, and of different sizes.Problems
Problem 1
With the compiler warning setting at "Default" (which I believe is the default), there is no warning printed about comparing signed and unsigned integers, which may not do what the user intended. At the "All" setting, we get the following, but I think most people would not see these warnings:
The following is the sample test case that generates this warning, which is hidden for most users. In this case, we have a test that passes on (8-bit) AVR, but fails on a 32-bit processor. You could argue that it's not ArduinoUnit's fault that the user wrote non-portable code. That's true, but I also think that dev tools should help users detect such conditions, and the fact that ArduinoUnit accepts this code (silently with the default "Default" compiler warnings setting) is somewhat unfortunate.
Running the above produces the following output on AVR:
and the following on both ARM and ESP8266:
Problem 2
This is a more serious problem than Problem 1. There's something wrong with the
assertLess()method on a 32-bit processor, because it produces the wrong result. (OtherassertXxx()methods may be affected, but I haven't tested them.)My expectation is that an
assertLess(a, b)should produce the same result as thisstatement. The following code shows that this expectation is true on AVR, but fails on a 32-bit processor.
I haven't spent much time digging through your new templatized versions of the
assertXxx()andcompareXxx()macros to figure out what's going on. Hopefully you will have better insights. Here's the code:On an AVR, this produces the following, which is expected, with a caveat:
The assertion result is correct (failed), but the assertion message is misleading, because it says that
(end - start=-1) < (limit=30)which is actually atrueexpression and should have passed. (My guess is that the there was an internal conversion to unsigned int, which caused the expression to befalse, even through it's printed to betrue.)On ARM and ESP8266, the
assertLess()fails, which is unexpected and I think incorrect result, because the if-statement evaluates totrue, so I would expect that theassertLess()should evaluate totrue, but it fails and produces this result:Comments
I suppose we could suggest to developers to avoid mixing signed and unsigned types, and mixing different integer sizes. I'm not sure that's practical. Maybe that suggestion works in a desktop or server environment, but in an embedded environment, I find myself using a variety of integer sizes to maximize static memory efficiency. And I find myself taking advantage of 2's complement unsigned rollovers quite often, to avoid having to use a larger integer type.
I also think the problems discussed here point to the advantages and necessity of running unit tests on the embedded environment itself, over running the tests on mocked out desktop or cloud environment. We can't really be sure something works until it compiles and runs on the target embedded environment.
Quick aside: I made the deliberate decision to try to avoid these problems in AUnit by requiring the two arguments in
assertXxx()to be identical (except for "string" types). AUnit produces a compile-time error (which unfortunately is somewhat lengthy and obtuse) if the types are not identical. I'm not saying that this was a better design choice, but just wanted to point out an alternative.