Hi Mark and Muhammad
On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote:
>
> > So it is not a problem with the MACRO, but where is it used ?
>
> > ret = sys_mseal(ptr, size);
> > FAIL_TEST_IF_FALSE(!ret);
>
> > Take this example, it would be
> > assert(!ret)
>
> The problem is that the macro name is confusing and not terribly
> consistent with how the rest of the selftests work. The standard
> kselftest result reporting is
>
> ksft_test_result(bool result, char *name_format, ...);
>
> so the result of the test is a boolean flag which is passed in. This
> macro on the other hand sounds like a double negative so you have to
> stop and think what the logic is, and it's not seen anywhere else so
> nobody is going to be familiar with it. The main thing this is doing is
> burying a return statement in there, that's a bit surprising too.
>
Thanks for explaining the problem, naming is hard. Do you have a
suggestion on a better naming?
> I'll also note that these macros are resulting in broken kselftest
> output, the name for a test has to be stable for automated systems to be
> able to associate test results between runs but these print
>
> ksft_test_result_fail("%s: line:%d\n", \
> __func__, __LINE__); \
> return; \
>
> which includes the line number of the test in the name which is an
> obvious problem, automated systems won't be able to tell that any two
> failures are related to each other never mind the passing test. We
> should report why things failed but it's better to do that with a
> ksft_print_msg(), ideally one that's directly readable rather than
> requiring someone to go into the source code and look it up.
>
I don't know what the issue you described is ? Are you saying that we
are missing line numbers ? it is not. here is the sample of output:
Failure in the second test case from last:
ok 105 test_munmap_free_multiple_ranges
not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573
ok 107 test_munmap_free_multiple_ranges_with_split
# Planned tests != run tests (106 != 107)
> A more standard way to write what you've got here would be to have the
> tests return a bool then have a runner loop which iterates over the
> tests:
>
> struct {
> char *name;
> bool (*func)(void);
> } tests[];
>
> ...
>
> for (i = 0; i < ARRAY_SIZE(tests); i++)
> ksft_test_result(tests[i].test(), tests[i].name);
>
> then the tests can just have explicit return statements and don't need
> to worry about logging anything other than diagnostics.
>
> Depending on how much you need to share between tests you might also be
> able to use kselftest_harness.h which fork()s each test into a separate
> child and allows you to just fault to fail if that's easier.
>
> > > We are writing unit tests in a test framework, let's use very well
> > > established industry practices please.
>
> Plus also the fact that we have a framework here...
>
> > > Also note that you don't even need to reinvent the wheel, there is a
> > > fully-featured test harness available in
> > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> > > EXPECT_xxx() helpers.
>
> > The EXPECT_xxx() doesn't take care of reporting though, or maybe it
>
> I rather think people would've noticed if the test harness was so broken
> that it was unable to report failures. If it is that broken we should
> fix it rather than open coding something else.
In general, I agree with those comments, but I would like to rely on
domain experts in test infra to recommend what to use, or is
acceptable.
In this case, I hope Muhammad, who reviewed this code in the first
place, can make recommendations on a replacement of this macro.
I would image the needs of something similar to FAIL_TEST_IF_FALSE is
common in selftest writing:
1> lightweight enough so dev can pick up quickly and adapt to existing
tests, instead of rewriting everything from scratch.
2> assert like syntax
3> fail the current test case, but continue running the next test case
4> take care of reporting test failures.
Thanks
-Jeff
