| Message ID | 0fa191715b236766ad13c5f786d8daf92a9a0cf2.1618388989.git.npache@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | kunit: Fix formatting of KUNIT tests to meet the standard | expand |
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
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 <matthieu.baerts@tessares.net> 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 <davidgow@google.com> 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 > <matthieu.baerts@tessares.net> 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 <matthieu.baerts@tessares.net> 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
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
Hi David, On Sat, Apr 17, 2021 at 6:24 AM David Gow <davidgow@google.com> 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
diff --git a/net/mptcp/Kconfig b/net/mptcp/Kconfig index a014149aa323..20328920f6ed 100644 --- a/net/mptcp/Kconfig +++ b/net/mptcp/Kconfig @@ -22,7 +22,7 @@ config MPTCP_IPV6 depends on IPV6=y default y -config MPTCP_KUNIT_TESTS +config MPTCP_KUNIT_TEST tristate "This builds the MPTCP KUnit tests" if !KUNIT_ALL_TESTS depends on KUNIT default KUNIT_ALL_TESTS diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile index a611968be4d7..dcce3cbd1f19 100644 --- a/net/mptcp/Makefile +++ b/net/mptcp/Makefile @@ -9,4 +9,4 @@ obj-$(CONFIG_INET_MPTCP_DIAG) += mptcp_diag.o mptcp_crypto_test-objs := crypto_test.o mptcp_token_test-objs := token_test.o -obj-$(CONFIG_MPTCP_KUNIT_TESTS) += mptcp_crypto_test.o mptcp_token_test.o +obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o diff --git a/net/mptcp/crypto.c b/net/mptcp/crypto.c index b472dc149856..a8931349933c 100644 --- a/net/mptcp/crypto.c +++ b/net/mptcp/crypto.c @@ -78,6 +78,6 @@ void mptcp_crypto_hmac_sha(u64 key1, u64 key2, u8 *msg, int len, void *hmac) sha256(input, SHA256_BLOCK_SIZE + SHA256_DIGEST_SIZE, hmac); } -#if IS_MODULE(CONFIG_MPTCP_KUNIT_TESTS) +#if IS_MODULE(CONFIG_MPTCP_KUNIT_TEST) EXPORT_SYMBOL_GPL(mptcp_crypto_hmac_sha); #endif diff --git a/net/mptcp/token.c b/net/mptcp/token.c index feb4b9ffd462..8f0270a780ce 100644 --- a/net/mptcp/token.c +++ b/net/mptcp/token.c @@ -402,7 +402,7 @@ void __init mptcp_token_init(void) } } -#if IS_MODULE(CONFIG_MPTCP_KUNIT_TESTS) +#if IS_MODULE(CONFIG_MPTCP_KUNIT_TEST) EXPORT_SYMBOL_GPL(mptcp_token_new_request); EXPORT_SYMBOL_GPL(mptcp_token_new_connect); EXPORT_SYMBOL_GPL(mptcp_token_accept);
Drop 'S' from end of CONFIG_MPTCP_KUNIT_TESTS inorder to adhear to the KUNIT *_KUNIT_TEST config name format. Fixes: a00a582203db (mptcp: move crypto test to KUNIT) Signed-off-by: Nico Pache <npache@redhat.com> --- net/mptcp/Kconfig | 2 +- net/mptcp/Makefile | 2 +- net/mptcp/crypto.c | 2 +- net/mptcp/token.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)