unittests/uri_parser: Rework tests to be more verbose #18734
Conversation
| char *simple_uri_19 = "coap:https:///R@[2008::1]:5own//R@[2008::1]:5own/?v=1"; | ||
| char *simple_uri_20 = "coaP:https://R/RZ[2001[8:01[8::1]:5o:1]:5oTMv=1"; | ||
| char *simple_uri_21 = "coap:https://R@////////////////7///v=1"; | ||
| char *simple_uri_22 = "coa[:https:////[2001:db5ow:5own/Ov=1"; |
There was a problem hiding this comment.
A ton of the old test vectors are just like "mh okay? What exactly is being tested?". If you got any ideas, please drop them so I can rename them from simple_uri to something more helpful.
There was a problem hiding this comment.
I guess @cgundogan is the best person to ask here.
| * The invalid uri are in the next test. The numbering represents the | ||
| * sequence / order in the old test. | ||
| */ | ||
| char *simple_uri_05 = "coap:https://RIOT:test@[fe80:db8::1%tap0]:5683/.well-known/core?v=1"; |
There was a problem hiding this comment.
The old tests used to also check every component in the result after parsing. This is no longer the case.
a1fd37b
to
6c24d62
Compare
There was a problem hiding this comment.
Some minor inline comments from a very rough review. Overall, the changes look well justified. In particular, the code gains readability and newly introduced cases extend the test scope of the parser. But I did not check if (i) existing test cases were translated properly, nor (ii) if "relevant" test cases have been removed.
The unit test still passes on native as well as nrf52840dk.
| char *simple_uri_03 = "https://riot-os.org:99/bar/foo"; | ||
| char *simple_uri_04 = "https://riot-os.org/"; | ||
|
|
||
| /* these are the old test vectors which contained valid uri. |
There was a problem hiding this comment.
Not sure if this go to the code bass (but nice for reviewing)
| char *failure_msg = "Failure: The uri_parser failed to reject an invalid uri."; | ||
| int expected_ret = -1; /* Indicates an invalid uri */ | ||
|
|
||
| /* these are the old test vectors containing invalid uri */ |
| (strncmp(actual_str, expected_str, actual_len) == 0)) { | ||
| return 0; | ||
| } | ||
| printf( |
There was a problem hiding this comment.
Just out of curiosity, you have actually looked at the output of this, right?
There was a problem hiding this comment.
Yes ofc.
It looks like this:
main(): This is RIOT! (Version: 2018.01-devel-26059-g6c24d-tests/uri_parser)
Help: Press s to start test, r to print it is ready
s
START
......
URI-Input: co ap:https://example.org/
Expected return: -1
Observed return: 0
uri_parser_tests._error_if_scheme_is_invalid (tests/unittests/tests-uri_parser/tests-uri_parser.c 371) Failure: The uri_parser failed to reject an invalid scheme.
.....
run 11 failures 1
| ret = uri_parser_process_string(&uri_res, test_vec[i]); | ||
| if (ret != expected_ret) { | ||
| /* At the moment, the uri_parser is not capable of detecting invalid schemes | ||
| *_print_return_expectation(test_vec[i], expected_ret, ret); |
There was a problem hiding this comment.
Do we have some kind of TODO tag so this doesn't get lost?
There was a problem hiding this comment.
Removed from the PR. I will add this test later as soon as the uri-parser gets an appropriate update :)
6c24d62
to
5f7f35c
Compare
|
Thanks for your review @PeterKietzmann addressed most of your comments and already marked the spelling errors as resolved. |
a05f9ab
to
49efb72
Compare
✅ frdm-k22f
✅ nucleo-f303re
✅ nucleo-f767zi
✅ nucleo-l152re
✅ nrf52dk
✅ remote-revb
✅ nucleo-f411re
✅ frdm-kw41z
✅ esp32-wroom-32
✅ arduino-due
|
Murdock results✔️ PASSED 49efb72 unittests/uri_parser: Rework tests to be more verbose
ArtifactsThis only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now. |
|
Somehow murdock didn't really run any tests huh... |
Hi! 👋
As prototyped and reviewed in #18629, this is the start of rewriting the unit tests of the uri_parser.
I believe it can safely be merged into RIOT there are however some things worth noting:
This is related work to #18702
The testing of the uri_parser remains far from being complete after this is merged.