Hi Nico,
On 14/04/2021 10:58, Nico Pache wrote:
> Drop 'S' from end of CONFIG_MPTCP_KUNIT_TESTS inorder to adhear to the
> KUNIT *_KUNIT_TEST config name format.
For MPTCP, we have multiple KUnit tests: crypto and token. That's why we
wrote TESTS with a S.
I'm fine without S if we need to stick with KUnit' standard. But maybe
the standard wants us to split the two tests and create
MPTCP_TOKEN_KUNIT_TEST and MPTCP_TOKEN_KUNIT_TEST config?
In this case, we could eventually keep MPTCP_KUNIT_TESTS which will in
charge of selecting the two new ones.
Up to the KUnit maintainers to decide ;-)
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
Hi Nico, Matthieu,
Thanks for going to the trouble of making these conform to the KUnit
style guidelines.
On Wed, Apr 14, 2021 at 5:25 PM Matthieu Baerts
<[email protected]> wrote:
>
> Hi Nico,
>
> On 14/04/2021 10:58, Nico Pache wrote:
> > Drop 'S' from end of CONFIG_MPTCP_KUNIT_TESTS inorder to adhear to the
> > KUNIT *_KUNIT_TEST config name format.
[Super nitpicky comment for this series: It's 'adhere', not 'adhear'.
It's not worth resending this out just to fix the spelling here, but
if you do another version, it might be worth fiing.]
>
> For MPTCP, we have multiple KUnit tests: crypto and token. That's why we
> wrote TESTS with a S.
So (as this patch series attests), there are a few different config
options which cover (or intend one day to cover) multiple suites, and
hence end in KUNIT_TESTS. Personally, I'd still slightly prefer TEST
here, just to have a common suffix for KUnit test options, and that's
what I was going for when writing the style guide.
That being said, it's also worth noting that there is an explicit
exemption for existing tests, so if you (for example) have a bunch of
automation which relies on this name and can't easily be changed,
that's probably more important than a lone 'S'.
> I'm fine without S if we need to stick with KUnit' standard. But maybe
> the standard wants us to split the two tests and create
> MPTCP_TOKEN_KUNIT_TEST and MPTCP_TOKEN_KUNIT_TEST config?
>
> In this case, we could eventually keep MPTCP_KUNIT_TESTS which will in
> charge of selecting the two new ones.
This is certainly an option if you want to do it, but personally I
wouldn't bother unless it gives you some real advantage. One thing I
would note, however, is that it's possible to have a per-subsystem
'.kunitconfig' file, so if you want to run a particular group of tests
(i.e., have a particular set of config options for testing), it'd be
possible to have that rather than a meta-Kconfig entry.
While there are some advantages to trying to have a 1:1 suite:config
mapping, there's isn't actually anything that depends on it at the
moment (or indeed, anything actively planned). So, in my view, there's
no need for you to split the config entry unless you think there's a
good reason you'd want to be able to build one set of tests but not
the other.
> Up to the KUnit maintainers to decide ;-)
To summarise my view: personally, I'd prefer things the way this patch
works: have everything end in _KUNIT_TEST, even if that enables a
couple of suites. The extra 'S' on the end isn't a huge problem if you
have a good reason to particularly want to keep it, though: as long as
you don't have something like _K_UNIT_VERIFICATION or something
equally silly that'd break grepping for '_KUNIT_TEST', it's fine be
me.
So, since it matches my prejudices, this patch is:
Reviewed-by: David Gow <[email protected]>
Thanks,
-- David
Hi David,
Thank you for your very clear reply!
On 15/04/2021 08:01, David Gow wrote:
> On Wed, Apr 14, 2021 at 5:25 PM Matthieu Baerts
> <[email protected]> wrote:
>> Up to the KUnit maintainers to decide ;-)
>
> To summarise my view: personally, I'd prefer things the way this patch
> works: have everything end in _KUNIT_TEST, even if that enables a
> couple of suites. The extra 'S' on the end isn't a huge problem if you
> have a good reason to particularly want to keep it, though: as long as
> you don't have something like _K_UNIT_VERIFICATION or something
> equally silly that'd break grepping for '_KUNIT_TEST', it's fine be
> me.
Indeed it makes sense: we don't need to split nor to have a meta-Kconfig
entry. We can then remove the extra 'S' and update our tests suite:
Reviewed-by: Matthieu Baerts <[email protected]>
I see that the whole series has been marked as "Not Applicable" on
Netdev's patchwork:
https://patchwork.kernel.org/project/netdevbpf/patch/0fa191715b236766ad13c5f786d8daf92a9a0cf2.1618388989.git.npache@redhat.com/
Like patch 1/6, I can apply it in MPTCP tree and send it later to
net-next with other patches.
Except if you guys prefer to apply it in KUnit tree and send it to
linux-next?
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
Hi Matt,
> Like patch 1/6, I can apply it in MPTCP tree and send it later to
> net-next with other patches.
> Except if you guys prefer to apply it in KUnit tree and send it to
> linux-next?
Given 1/6 is going to net-next, it makes sense to send this out that
way too, then, IMHO.
The only slight concern I have is that the m68k test config patch in
the series will get split from the others, but that should resolve
itself when they pick up the last patch.
At the very least, this shouldn't cause any conflicts with anything
we're doing in the KUnit tree.
Cheers,
-- David
Hi David, Nico,
On 17/04/2021 06:24, David Gow wrote:
> Hi Matt,
>
>> Like patch 1/6, I can apply it in MPTCP tree and send it later to
>> net-next with other patches.
>> Except if you guys prefer to apply it in KUnit tree and send it to
>> linux-next?
>
> Given 1/6 is going to net-next, it makes sense to send this out that
> way too, then, IMHO.
Great!
Mat sent this patch to net-next and David already applied it (thanks!):
https://git.kernel.org/netdev/net-next/c/3fcc8a25e391
> The only slight concern I have is that the m68k test config patch in
> the series will get split from the others, but that should resolve
> itself when they pick up the last patch.
I see. I guess for this MPTCP patch, we are fine because
MPTCP_KUNIT_TESTS was not used in m68k config.
> At the very least, this shouldn't cause any conflicts with anything
> we're doing in the KUnit tree.
Thanks for having checked!
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
Hi David,
On Sat, Apr 17, 2021 at 6:24 AM David Gow <[email protected]> wrote:
> > Like patch 1/6, I can apply it in MPTCP tree and send it later to
> > net-next with other patches.
> > Except if you guys prefer to apply it in KUnit tree and send it to
> > linux-next?
>
> Given 1/6 is going to net-next, it makes sense to send this out that
> way too, then, IMHO.
> The only slight concern I have is that the m68k test config patch in
> the series will get split from the others, but that should resolve
> itself when they pick up the last patch.
I can apply the m68k test config patch when all other parts have
entered mainline. Note that I would have made the same changes
myself anyway, on -rc1 defconfig refresh.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds