Message-ID: <20251028162956.6011e15d@kernel.org>
Date: Tue, 28 Oct 2025 16:29:56 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 shuah@...nel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring
 test with a drv-net one

On Tue, 28 Oct 2025 03:05:03 +0100 Andrew Lunn wrote:
> On Mon, Oct 27, 2025 at 05:15:39PM -0700, Jakub Kicinski wrote:
> > On Mon, 27 Oct 2025 20:46:00 +0100 Andrew Lunn wrote:  
> > > We expect failure to leave the configuration unchanged. So i would
> > > actually do:
> > > 
> > > try:
> > > 	before = get()
> > > 	set()
> > > except:
> > > 	after = get()
> > > 	fail(after != before)  
> > 
> > Please allow me to introduce you to the magic of defer() ;)  
> 
> That is why i don't like magic, especially in tests which have no
> documentation of the expected results. For me, tests should be dumb,
> often boringly repetitive, and at least 50% comments, explaining what
> is being tested, what the expected outcome is, and most importantly,
> why that is the expected outcome.

We actively avoid creating framework-y stuff.

defer() is one of the few things we added. Single config change is fine
but undoing a sequence of actions quickly becomes gnarly. And defer()
itself is very straightforward.. once you know about it ;)

https://github.com/linux-netdev/nipa/wiki/Guidance-for-test-authors

> > This registers a command to run after the test completely exits:
> > 
> > +    defer(cfg.eth.channels_set, ehdr | restore)
> >   
> > > Also, does nlError contain the error code?
> > > 
> > >         fail(e.errcode not in (EINVAL, EOPNOTSUPP))
> > > 
> > > It would be good to detect and fail ENOTSUPP, which does appear every
> > > so often, when it should not.  
> > 
> > Dunno, checkpatch warns about ENOTSUPP. I don't that think checking 
> > for funny error codes in every test scales :(  
> 
> How about in the nlError constructor? That gives you a single
> location, and you can accept EINVAL, EOPNOTSUPP, ENODEV, ENOMEM, maybe
> ETOOBIG. Cause the test to fail for everything else. If anybody
> reports test failures with other errno values, the list can be
> expanded, if they are sensible.

Maybe it's just my subjective preference but I think we need to be
judicious about what we test. I see no value in checking errno here.
This is mostly a "crash test", IOW must common issues it will find
will be crashes, WARN()s or broken device. Nothing subtle.

> > > Is 0 ever valid? I would actually test 0 and make sure it fails with
> > > EINVAL, or EOPNOTSUPP. Getting range checks wrong is a typical bug, so
> > > it is good to test them. The happy days cases are boring because
> > > developers tend to test those, so they are hardly worth testings. Its
> > > the edge cases which should be tested.  
> > 
> > I believe that 0 is a valid settings. I don't have much experience with
> > devices which support it. But presumably using 0 to disable mini/jumbo
> > rings would make sense for example? And max validation is done by the
> > core so nothing interesting to explore there at the driver level :(  
> 
> Looking at the code, it seems to cost nothing to actually test 0, if
> you say it could be valid. That might also find an off-by-one error,
> if it causes something to go negative/large positive.

Okay. But for the record I'd strongly prefer it the level of nit
picking was correlated with reviewer's authorship of tests :/

Powered by blists - more mailing lists