draft: uri_parser: Proposal to rewrite the unittest

[Teufelchen1]

Hi 👋

The current testing of the uri_parser is inadequate and an obstacle when working on changes.

This PR proposes a new approach. It follows some basic guidelines, which typically lead to good tests.

Short list of basic guidelines

1. When a test fails...reparability:
   There are two reasons for test failures:
   A - There is a bug or a problem in the system under test. This is exactly what tests are written for - alert that something is broken in this system.
   B - The system under test is fine but the test itself has an issue. The test was most likely written incorrectly.

   A good test is VERY clear about its own reason for existing. When it fails, the RIOT developer can immediatly tell in which of the two categories this failure belongs. This not only safes time but is also much more fun to fix.
   This is the reason why the proposed tests have such verbose names and extra debug output on failure.

2. A new user comes along...helpfulness:
   There are two main reasons to look at existing tests:
   A - They failed.
   B - You are new to the system under test and where looking for an example on how to use it. Maybe the documentation was unclear. Or the system does not work as expected even though you are confidend to use it correctly.

   Tests, as all code, is much more often read than written. When writing them, they should prioritise readability over elegance. (I personally think that applies to all code, always.) In tests, you can't claim execution speed or resource constrains as an excuse for poor readability.
   This is the reason why the proposed tests clearly show how to invoke the uri_parser and what to expect from its results. Making it easy to understand the usage and behavoir of the system under test.

3. You discovered a bug and therefor a test hole...extensibility:
   When ever a bug is found in a system that has tests, the tests were incomplete. The next step is to fix that hole. Tests should be written in a way that makes it easy, simple and obvious how and where to add a new test case. This is especially problematic with the current tests. You'll need extensive C knowledge and time to understand all the macros and the test itself. It absolutly not clear where you would put your hole covering tests.
   This proposal features easy to understand patterns which one can replicate to add a new test vector or implement a new test.

4. Time flies by...maintainability:
   Code ages, developers come and go. It is easy to imagine that the author of a test has left the RIOT team years ago - when it suddendly fails. Understanding, expectations and requierments of the system under test might have changed. Without the author, the tests will need to explain themself. Hence the clarity of the tests is so important. If you don't know how or why a test is performed, neither do you know why it failed. Being verbose and clear is the key element for a maintainable test.
   In this proposal, maintainability is archived by being concise and complete. They are complete because every testcase includes all information needed by the reader to understand how/why the result of the uri_parser occures (here, typically the input uri and why this specific uri was choosen) and how/why we expect it to be a specific value. They are concise this they are scarce, only containing the absolute minimum of code requiered to run the specific test. Thats the exact opposite of the current tests, which are as unconcise as possible, containing so many unclear loops, macros and global references that you might want to write tests for the test.

I could go on and on about testing. About not testing methodes but behaviors instead. Or why one shouldn't put logic, complex concatinations, loops etc into tests. Hopefully this way to long PR description is already enough to convice you that the current state of testing is inadequate.

Please let me know if you have any questions, ideas or criticism. Also make sure to show your approval if you believe I should push this forward & replace the current uri_parser tests for real.

Thanks for comming to my RIOT-Summit talk

[miri64]

Please let me know if you have any questions, ideas or criticism.

Github bot has some ;-). Can you please go through the annotations in the "Files" tab and address them. Hope, I will find some time tomorrow to look at this.

[miri64]

The commit message should provide some scope.

[miri64]

In general I agree that this is an improvement over the currently rather esoteric unittests for uri_parser. Is it maybe possible to have it reflected in the Git history which test replaces which test vector for future reference instead of just adding a complete new test suite?

[miri64]

Why not use TEST_ASSERT_MESSAGE for this?

[Teufelchen1]

One could do that but I prefer it this way:

• more precise and clear, you'll need less knowledge / experience with testing to understand it
• is less depended on embUnit - which is unmaintained for a couple years now. For me, it is unclear how invested RIOT is into embUnit but I'm (mentally ;)) prepared to remove it from RIOT someday.
• It "feels" better to me.

(its not a dealbreaker for me to adapt TEST_ASSERT_MESSAGE instead)

[benpicco]

How much maintenance does a test framework need? 😉
I think it would be a lot of wasted effort to re-write all the tests…

[Teufelchen1]

@MrKevinWeiss Give it a review please! 🥺👉👈

[Teufelchen1]

Is it maybe possible to have it reflected in the Git history which test replaces which test vector for future reference instead of just adding a complete new test suite?

I would also rather replace the old one. I do not think it is worth mapping each single vector to the new test but for some that definitely possible without that much extra effort. - I deliberately added a new file for the draft for an easier review.

The commit message should provide some scope.

well yes but no. I don't think this PR is going to be the one getting merged into RIOT. For me, this about getting feedback before doing actual work on the tests.

[miri64]

If this is about testing in general and how to write good tests, I think an additional point also should be coverage. Yes, currently this is hard to test, but there is #17000.

[benpicco]

If you want to re-write this particular test, sure go ahead!
tests-uri_parser.c is full of macro-gore, if you get the same coverage with something more readable, I'd be very happy.

[MrKevinWeiss]

First, I think many people won't mind getting the tests improved, many times the focus was not on the testing but more on the feature. This has improved but there is still lots of legacy tests that can really be improved.

The overall concept is sound, write understandable (simple) tests that also indicate the writers intent. Assuming the test coverage is equal or better and there isn't too much bloat that would make some tests not longer run on some boards, then there should be no blocker here.

Just to clarify my understanding of this, tests are good, but if a test isn't understandable it can make the developer experience more challenging (I did something and now the tests are failing but I have no idea about why). Thus, some care should be taken when adding tests.

In tests, you can't claim execution speed or resource constraints as an excuse for poor readability.

To a degree this is true, we do struggle with some tests (or more build time) with CI and some boards cannot run tests as they do not fit in. These are somewhat edge cases and in the majority of cases I would think that readability should be the most important.

When ever a bug is found in a system that has tests, the tests were incomplete. The next step is to fix that hole. Tests should be written in a way that makes it easy, simple and obvious how and where to add a new test case.

Sometimes the feature implementation is done in a what that prevents this possibility...

You'll need extensive C knowledge and time to understand all the macros and the test itself.

There is a range of C knowledge amongst the RIOT developers. Though I don't think we should limit the complexity of tests to the bare minimum, we should aim to make thing as simple as possible.

Or why one shouldn't put logic, complex concatinations, loops etc into tests.

Agreed, but simple loops and logic (as long as they are around the minimum RIOT developer level) can be used.

[MrKevinWeiss]

If you want you can close in favor of #18734