Message-ID: <e0f440b0-5a45-4218-8c51-27f848c0617b@lucifer.local>
Date: Thu, 17 Oct 2024 20:00:10 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: Muhammad Usama Anjum <usama.anjum@...labora.com>,
        akpm@...ux-foundation.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org, pedro.falcato@...il.com,
        willy@...radead.org, broonie@...nel.org, vbabka@...e.cz,
        Liam.Howlett@...cle.com, rientjes@...gle.com, keescook@...omium.org
Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote:
> On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> > > Hi Lorenzo and Muhammad
> > >
> > > Reviving this thread since the merging window is closed and we have
> > > more time to review /work on this code in the next few weeks.
> > >
> > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@...omium.org> wrote:
> > > >
> > > > Hi Lorenzo
> > > >
> > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > > > <lorenzo.stoakes@...cle.com> wrote:
> > > > >
> > > > >
> > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point
> > > > > too - I may be missing something, but I cannot for the life me understand
> > > > > why we have to assert negations only, and other self tests do not do this.
> > > > >
> > > > My most test-infra related comments comes from Muhammad Usama Anjum
> > > > (added into this email), e.g. assert is not recommended.[1] ,
> > > >
> > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> > > >
> > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
> > >
> > > Muhammad Usama Anjum doesn't want assert being used in selftest (see
> > > [1] above), and I quote:
> > > "We don't want to terminate the test if one test fails because of assert. We
> > > want the sub-tests to get executed in-dependent of other tests.
> > >
> > > ksft_test_result(condition, fmt, ...);
> > > ksft_test_result_pass(fmt, ...);"
> > >
> > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> > > replacement of assert.
> > >
> > > Please let me know if you have questions on this and Muhammad might
> > > also help to clarify the requirement if needed.
> > >
> > > Thanks
> > > -Jeff
> >
> > Right this is about not failing the test i.e. equivalent of an expect
> > rather than an assert, which makes sense.
> >
> > What I'm saying is we should have something more like
> >
> > EXPECT_TRUE()
> > EXPECT_FALSE()
> >
> > etc.
> >
> > Which would avoid these confusing
> >
> >         FAIL_TEST_IF_FALSE(!expr)
>
> FAIL_TEST_IF_FALSE(expr) is the right way to use this macro.

But you don't only test position conditions, you also test negative ones.

'Fail test if false false thing' is really confusing and hard to read.

I struggle to understand your tests as a result.

I understand 'fail test if false' is expressive in a way, but it's really hard
to parse.

Obviously it's also misleading in that you're saying 'fail the test _later_
if false', which I hadn't even realised...

It's well established in basically all normal test suites that:

* assert = fail test _here_ if this fails (actually a valid thing to do if
           you assert something that means the test simply cannot
           reasonably continue if that condition is false).
* expect = the test will now fail, but carry on.

I mean you work for a company that does this :) [0] this is a very well
established precedent.

[0]:https://github.com/google/googletest

>
> It is same syntax as assert(expr), e.g:
>
> man assert(expr)
>        assert - abort the program if assertion is false
>
> FAIL_TEST_IF_FALSE is a replacement for assert,  instead of aborting
> the program, it just reports failure in this test.

So doesn't at all do what assert does, because that _does_ terminate
execution on failure...

We are writing unit tests in a test framework, let's use very well
established industry practices please.

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.

I've used it extensively myself and it works well.

I'd basically suggest you use that. Though moving existing tests to that
would be some churn.

On the other hand I really can't accept patches which are totally
unreadable to me, so you'll need to fix this one way or another, and the
churn is worth it as a one-time cost to be honest.

>
> Is this still confusing ?
> (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax
> of assert is well known.)

It's a super misleading name as it says nothing about _WHEN_ the test
fails. Also the syntax of assert() may be well known but you don't call
this function assert, you don't reference assert anywhere, and you don't do what
assert() does so, you know, That's not a great example.

The semantics of unit test frameworks are very well known, and already
implemented for you, and also do not require you to do unreadable double
negations for no reason, so let's use those please.

>
>
> >
> > Things.
> >
> > Hopefully that's clear? Thanks!

Powered by blists - more mailing lists