Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4678 bytes --]

On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote:
> On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:

> > Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
> > line:2573" which automated systems aren't going to be able to associate
> > with the passing "test_munmap_free_multiple_ranges_with_split", nor with
> > any failures that occur on any other lines in the function.

> I see. That will happen when those tests are modified and line number
> changes. I could see reasoning for this argument, especially when
> those tests are flaky and get updated often.

> In practice, I hope any of those kernel self-test failures should get
> fixed immediately, or even better, run before dev submitting the patch
> that affects the mm area.

That's not the entire issue - it is also a problem that the test name
is not the same between passes and failures so automated systems can't
associate the failures with the passes.  When a test starts failing they
will see the passing test disappear and a new test appear that has never
worked.  This will mean that for example if they have bisection support
or UI for showing when a test started regressing those won't work.  The
test name needs to be stable, diagnostics identifying why or where it
failed should be separate prints.

Actually, prompted by the comments below about test variants I've now
run the test and see that what's in -next is also broken in that it's
running a lot of the tests twice with sealing enabled or disabled but
not including this in the reported test name resulting in most of the
tests reporting like this:

   ok 11 test_seal_mprotect
   ok 12 test_seal_mprotect

which is also going to confuse automated systems, they have a hard time
working out which instance is which (generally the test numbers get
ignored between runs as they're not at all stable).  The test names need
to roll in the parameterisation:

   ok 11 test_seal_mprotect seal=true
   ok 12 test_seal_mprotect seal=false

(or something, the specific format doesn't matter so long as the names
are both stable and distinct).

> Having line number does help dev to go to error directly, and I'm not
> against filling in the "action" field, but you might also agree with
> me, finding unique text for each error would require some decent
> amount of time, especially for large tests such as mseal_test.

In these situations if it's a typical Unix API function setting errno
that failed I tend to end up writing diagnostics like:
	
	ksft_perror("open()")

possibly with some of the arguments included as well, or something
equivalently basic for other kinds of error.  This is fairly mindless so
quick and easy to do and more robust against line number slips if you're
not looking at exactly the same version of the code, sometimes it's even
enough you don't even need to look at the test to understand why it's
upset.

> > Honestly this just sounds and looks like kselftest_harness.h, it's
> > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
> > for asserts.  The main gotchas with it are that it's not particularly

> OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture.

> If I  switch to test_fixture, e,g, using TEST(test_name)

> how do I pass the "seal" flag to it ?
> e.g. how do I run the same test twice, first seal = true, and second seal=false.

>         test_seal_mmap_shrink(false);
>         test_seal_mmap_shrink(true);

That looks like fixture variants to me, using those with
kselftest_harness.h will also fix the problem with duplicate test names
being used since it generates different names for each instance of the
test.  Something like:

FIXTURE(with_seal)
{
};

FIXTURE_VARIANT(with_seal)
{
	bool seal;
};

FIXTURE_VARIANT_ADD(with_seal, yes)
{
	.seal = true,
};

FIXTURE_VARIANT_ADD(with_seal, no)
{
	.seal = false,
};

FIXTURE_SETUP(with_seal)
{
}

FIXTURE_TEARDOWN(with_seal)
{
}

then a bunch of tests using that fixture:

TEST_F(with_seal, test_seal_mmap_shrink)
{
	if (variant->seal) {
		/* setup sealing */
	}

	...
}

TEST_F(with_seal, test_seal_mprotect)
{
	if (variant->seal) {
		/* setup sealing */
	}

	...
}

You don't need to actually set up anything in your fixture, but you do
need to have setup and teardown functions so the framework can emit
required boilerplate.  The gcs-locking.c test I recently added in -next 
is an example of a similar thing where we just need the variants,
there's no actual fixture.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]