Good Morning all,
my real life distraction is mostly gone and I can be more active in developing and
reviewing again. From my point of view Sysmocom and me have responded to all Gerrit
reviews and we are waiting for the next iteration of the patchset. Before giving a
high-level point of view of orders and dependencies I would like to say some words about
testing.
(Automatic Unit-)testing is crucial. Sometimes it is the easiest way to test a behavior,
it allows to hit/test error paths but are hit less frequently. Combined with regular
execution of the tests and forbidding to merge code that breaks tests, features that
worked once and have a good test will continue to work. It is the key for being able to
move forward quickly.
For many patches I reviewed test + feature or test + bugfix were in separate commits and
then the order is problematic. From my point of view there are only a few
"right" combinations. Let me try to explain:
Assume some code is doing "if (res) " instead of "if (!res)" and after
long debugging we have found that single line (and our code is full of printf now) and it
is a path that has not been systematically tested yet. To move forward and create a
patchset we should:
* Add a test case that triggers the broken behavior. With a xUnit framework we would add
an expected failure, with our approach we either capture the broken output, comment the
OSMO_ASSERT or revert the OSMO_ASSERT condition.
(See libosmocore
* Let's say that if is already in code like:
if (a)
if (b)
if (c)
if (d)
and we would like to re-factor it to be more readable. Then please do the refactoring
_before_ applying the bugfix. The test output/result should not change as it is a
refactoring.
* Finally apply the fix. For bugfixes try to make them as small and clear as possible.
Let's say even if you don't like the algorithm used, fix it. As part of the fix
the test assertions can be enabled and the output change. The smaller the fix, the better.
See libosmocore 6ac70a41ee4cc9f3f286fb6b8f397215590fc2ac for such an example.
* If the algorithm is indeed bad now is the time to change it. It should just change the
(refactored) method, test output and result should be stable and maybe add more tests as
the new code is easier to test.
In general my preferred way:
New feature => one commit to add feature + test
Bugfix => First test, then bugfix+test output/result update
thank you
holger