[ntp-GSoC] Unity: agreeing to some convention, code review

Tomasz Flendrich t.flendrich at gmail.com
Sat Jun 27 05:23:15 UTC 2015


Hi guys,

1. Helper functions in Unity
As far as I remember, on IRC someone said that if we are writing some tests
and want a helper function, it should just return a bool which we test with
TEST_ASSERT_EQUAL(helper_function_call(...) == 1).
The problem with that approach is that we don't really know what happened -
which "assertion" failed. If this helper function has 1 assert only, it
still isn't good for us, because we don't know what was asserted and what
was the expectation.
For example:
bool helper_function(int year, int expectedYear)
{
return year - 1 == expectedYear;
}
this is the assertion we have to use and the message we get when it fails:
TEST_ASSERT_TRUE(helper_function(arg1, expected))
filename.c:test_something:FAIL: Expected TRUE Was FALSE

what about this approach:
bool helper_function(int year, int expectedYear)
{

char msg[100];
msg = information about year and expectedYear;
TEST_ASSERT_EQUAL_MESSAGE(expectedYear, year - 1, msg);
}
now when this assertion fails, we have a message saying:
filename.c:35:test_something:FAIL: Expected 2015 Was 1000. helper_function
was called with year=1001, expectedYear=2015.

I think that the latter is way better.
Now imagine if we have a few assertions in every test, which is absolutely
possible, and sometimes mandatory (for example if we do some pointer stuff
and want to check what lies under some pointer, we first check if it's not
pointing to NULL). Using the second approach we know exactly which
assertion failed (we know the line in which that assertion was) and we also
know what arguments were used in that helper function. Of course we may add
some additional information too.

I used that approach in the file test/libntp/refidsmear.c. You can access
it from Harlan Stenn:
~stenn/ntp-stable-unity/tests/libntp/refidsmear.c
I really like the way it works, because if any assertion fails, we see
immediately know everything we need to.



2. Which compare function to use?
Let's say that I want to compare two chars, char1 and char2, and test if
they are equal. The first thing that comes to mind is:
TEST_ASSERT_EQUAL_UINT8(char1, char2).
While it's usually right, technically the standard says that "char" type
should have at least 8 bits. And it's not only theoretical: I found some
information about some machine that actually used bigger chars.

And a quote from Wikipedia:
>Various implementations of C and C++ define a byte as 8, 9, 16, 32, or 36
bits
https://en.wikipedia.org/wiki/Byte

The POSIX standard requires that CHAR_BIT == 8, but NTP can run on
something that isn't POSIX-compliant (e.g. embedded platforms).

What if we compare something that is 16 bits in length and accidentally use
8 bit assertion? It may compile, the test may pass and we would be happy,
and bugs would be undiscovered while being "covered" by our code.
And there are examples where the size of the variable we currently compare
isn't obvious, because it's hidden behind many C macros, but we are sure
that it doesn't exceed 64 bits. It's really easy to make a mistake.
What do you think about using TEST_ASSERT_EQUAL (or TEST_ASSERT_EQUAL_UINT)
in those cases, even if we compare something shorter?



3. Should we test a helper function input?

Again, a real life example is in:
~stenn/ntp-stable-unity/tests/libntp/refidsmear.c

Is that assertion that checks if "es" variable is not NULL useful there?
The test will definitely crash if we give a NULL there, but this way we
test not our production code, but our tests. If we do that, maybe we do
more: we test that the input is not only not NULL, but that its length
doesn't exceed some_number. Then maybe test that the input string doesn't
have letters in it, but only numbers and one dot? When should we stop?



4. Code review:
I added three columns, one for each of us, to our docs with the summary of
converted tests. After you complete reviewing some test, please mark it
there. Also post any notes there too.
If during code review you encounter anything that may, but doesn't have to
be done differently and want to further discuss it, please respond to this
email.



5. Important info from Harlan regarding NTP convention
In every curly braces (functions, loops etc.), we *must* have variable
declarations first, then code that executes something.
We have to do that because we support compilers that require it.

On the other hand, stuff like this is okay: (from tests/libntp/caltontp.c)

void test_WraparoundDateIn2036(void) {
struct calendar input = {2036, 0, 2, 7, 6, 28, 16};
u_long expected = 0UL;
TEST_ASSERT_EQUAL_UINT(expected, caltontp(&input));
}

I glanced at our converted tests and noticed one place where this order was
not preserved. There are perhaps more places like this, so *please* handle
it when doing code review.



Please let me know if you have any insight.

Regards,
Tomasz Flendrich


More information about the GSoC mailing list