From: John Johansen <firstname.lastname@example.org> To: Kees Cook <email@example.com>, Luis Chamberlain <firstname.lastname@example.org> Cc: Brendan Higgins <email@example.com>, Alan Maguire <firstname.lastname@example.org>, Matthias Maennich <email@example.com>, shuah <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, Iurii Zaikin <email@example.com>, David Gow <firstname.lastname@example.org>, Theodore Ts'o <email@example.com>, Linux Kernel Mailing List <firstname.lastname@example.org>, email@example.com, KUnit Development <firstname.lastname@example.org>, "open list:KERNEL SELFTEST FRAMEWORK" <email@example.com>, Mike Salvatore <firstname.lastname@example.org> Subject: Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack Date: Wed, 30 Oct 2019 18:37:24 -0700 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <201910301205.74EC2A226D@keescook> On 10/30/19 12:09 PM, Kees Cook wrote: > On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote: >> On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote: >>> With that, I think the best solution in this case will be the >>> "__visible_for_testing" route. It has no overhead when testing is >>> turned off (in fact it is no different in anyway when testing is >>> turned off). The downsides I see are: >>> >>> 1) You may not be able to test non-module code not compiled for >>> testing later with the test modules that Alan is working on (But the >>> only way I think that will work is by preventing the symbol from being >>> inlined, right?). >>> >>> 2) I think "__visible_for_testing" will be prone to abuse. Here, I >>> think there are reasons why we might want to expose these symbols for >>> testing, but not otherwise. Nevertheless, I think most symbols that >>> should be tested should probably be made visible by default. Since you >>> usually only want to test your public interfaces. I could very well >>> see this getting used as a kludge that gets used far too frequently. >> >> There are two parts to your statement on 2): >> >> a) possible abuse of say __visible_for_testing > > I really don't like the idea of littering the kernel with these. It'll > also require chunks in header files wrapped in #ifdefs. This is really > ugly. > >> b) you typically only want to test your public interfaces > I would disagree with this. Helper/lib functions benefit from testing, and ensuring they work as expected. Having tests on these helps catch errors when you fix bugs or make improvements. If you want its indirect testing of public interfaces. > True, but being able to test the little helper functions is a nice > starting point and a good building block. > yeah its a nice building block > Why can't unit tests live with the code they're testing? They're already > logically tied together; what's the harm there? This needn't be the case > for ALL tests, etc. The test driver could still live externally. The > test in the other .c would just have exported functions... ? > they can, its my preference too. Or if the tests must live separate I don't even mind the abomination of including a test .c that contains the tests for the private fns only. In the end though, I just want to see more unit testing and will happily take what ever the community decides is the way to go.
next prev parent reply other threads:[~2019-10-31 1:37 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-18 0:18 Brendan Higgins 2019-10-18 0:33 ` Iurii Zaikin 2019-10-30 18:59 ` Kees Cook 2019-11-06 0:35 ` Brendan Higgins 2019-11-06 0:37 ` Brendan Higgins 2019-10-18 0:43 ` Brendan Higgins 2019-10-18 16:25 ` Theodore Y. Ts'o 2019-10-18 21:41 ` Brendan Higgins 2019-10-30 19:02 ` Kees Cook 2019-10-31 9:01 ` Brendan Higgins 2019-10-18 12:29 ` Luis Chamberlain 2019-10-19 12:56 ` Alan Maguire 2019-10-19 18:36 ` Luis Chamberlain 2019-10-24 0:42 ` Brendan Higgins 2019-10-24 10:15 ` Luis Chamberlain 2019-10-30 19:09 ` Kees Cook 2019-10-30 20:11 ` Iurii Zaikin 2019-10-31 1:40 ` John Johansen 2019-10-31 9:33 ` Brendan Higgins 2019-10-31 18:40 ` Kees Cook 2019-11-05 16:43 ` Mike Salvatore 2019-11-05 23:59 ` Brendan Higgins 2019-10-31 1:37 ` John Johansen [this message] 2019-10-31 9:17 ` Brendan Higgins 2019-11-01 12:30 ` Alan Maguire 2019-11-05 23:44 ` Brendan Higgins
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).