unittests: add fletcher* tests #4938
Conversation
LudwigKnuepfer
added
Area: tests
miri64
removed
the
CI: ready for build
|
Travis is already overloaded as is. Please don't set the CI label before it was ACK'd |
| } | ||
|
|
||
| { | ||
| /* XXX: not verified with externa implementation yet */ |
LudwigKnuepfer
force-pushed
the
pr/fletcher-unittest
branch
2 times, most recently
from
8ef0d25
to
be5cbfd
Compare
| uint16_t expect = 0xFFFF; | ||
|
|
||
| TEST_ASSERT(calc_and_compare_checksum(buf, sizeof(buf) - 1, expect)); | ||
| } |
There was a problem hiding this comment.
What kind of syntax is this? Why does this function have more than one assert?
There was a problem hiding this comment.
Syntax is C I hope.
The function has several asserts because it tests several values.
There was a problem hiding this comment.
Should be several functions then.
There was a problem hiding this comment.
And I still don't get the purpose of the additional curly brackets.
There was a problem hiding this comment.
Will split once the test logic is acknowledged
There was a problem hiding this comment.
And I still don't get the purpose of the additional curly brackets.
Scoping in order to reuse the variable name. I.e. readability.
There was a problem hiding this comment.
Will split once the test logic is acknowledged
I think this is done now.
There was a problem hiding this comment.
Yes, will tend to shortly.
|
@gebart - can you ACK the logic? |
| 0x00, 0x00, 0x00, 0x00, | ||
| 0x1A, 0x1A, 0x1A, 0x1A, | ||
| }; | ||
| uint32_t expect = fletcher32((const uint16_t *) buf0, sizeof(buf0)/2); |
There was a problem hiding this comment.
/ sizeof(uint16_t) instead of /2
|
ACK for the logic. I'm missing the precomputed expected value for fletcher32, but at least there's a TODO note there. |
|
Tested on native and mulle, tests pass, I guess that's good. |
|
One minor comment regarding literal 2, see above. You may squash immediately |
jnohlgard
added
the
CI: ready for build
Like outlined in the PR description I had trouble finding a good reference. Your old PR only has 16 bit reference values as well... I guess I could add a test for a known reference value that this implementation has produced, so we at least know it does not change. |
|
btw, I just realized that it might be a good idea for the fletcher tests to have a test input vector which accumulates enough to "wrap around" the modulo part of the algorithm. The current test vector is quite short and containing small numbers, could you make another test vector which is longer and with some greater numbers. https://en.wikipedia.org/wiki/Fletcher%27s_checksum#cite_note-3 If you want we can merge this now and add another test vector in a separate PR. |
jnohlgard
added
CI: ready for build
|
ping @LudwigKnuepfer please squash |
|
@gebart should I squash without addressing the comments first? |
LudwigKnuepfer
force-pushed
the
pr/fletcher-unittest
branch
from
be5cbfd
to
07f5588
Compare
LudwigKnuepfer
force-pushed
the
pr/fletcher-unittest
branch
from
07f5588
to
ef25f32
Compare
|
Just squashed the old fixup commit - may be merged as is. I side-changed the same style "problem" in crc16-ccitt while I was at it. |
|
OK, let's merge this. |
|
Added an issue as a reminder to update the tests with more input vectors to improve coverage of the algorithm |
The fletcher32 checksum does not match what the tool referenced in the fletcher16 test produces. I guess this is due to a common (I looked at various implementations on GitHub et al) implementation deviation where 8 bit words are summed instead of 16 bit words.