Closed david-a-wheeler closed 9 years ago
david-a-wheeler
commented
9 years ago Here's a first cut:
Tests are added for new functionality. Whenever major new functionality is added to the program, tests of it MUST be added to the automated test suite. This MUST be documented in the instructions for change proposals, and there MUST be evidence that such tests are being added in the most recent major changes to the project.
gregkh
commented
9 years ago We (the kernel developers) discussed making this an "official" rule last year at the kernel summit, and that was shot down, so it is currently an "unofficial but highly recommended" rule at the moment. The kernel developers only have one real "official rule", "don't break userspace" and the odds of us making more rules than that is going to be hard, given the way we are organized.
Your proposal is great, but the kernel would not be able to pass it (support for new hardware is almost impossible to add to any type of test suite.) But this is fine, we don't have to pass all of the recommendations in this document.
david-a-wheeler
commented
9 years ago The kernel developers only have one real "official rule", "don't break userspace" and the odds of us making more rules than that is going to be hard, given the way we are organized.
Fair enough. However, the Linux kernel project already has a huge number of guidelines for contributions: https://www.kernel.org/doc/Documentation/development-process/ Perhaps we could modify this criterion so it's clear that the Linux kernel could simply add something like this to their own guidelines: "If you are adding major new functionality, please also provide test case(s), typically in one or more of the following: make kselftest, xfstests. the Linux test project, or Autotest." (Tweak text to taste.)
Your earlier point is still valid: it's difficult to create true test suites for a kernel. Another approach would be to simply modify this criterion so it doesn't apply to kernels.
We don't have to pass all of the recommendations in this document.
Well... I've currently written that text as a requirement (MUST) for the lowest-level badge, not just as something RECOMMENDED. We could change it to be RECOMMENDED instead, of course.
No law says that the criteria must be met by the Linux kernel. Still, I'd like these criteria to be met by the Linux kernel (possibly with changes agreed to by the kernel developers, if they agree they're good ideas)... or be confident that the requirements should be met. It's perfectly reasonable to suspect that if the criteria aren't met by many OSS projects, and will not be, then perhaps the criteria are the problem.
Our plan is to later create "higher level" badges, and perhaps the Linux kernel can't meet those (perhaps NO kernel could), but that's a different question.
So - which option(s) make sense?
gregkh
commented
9 years ago Because of hardware issues, the kernel can never say "all new features must have tests". What we can do, is encourage them, and for things like new syscalls, very strongly encourage them.
Again, this is what we are doing, and again, we agreed last year that we would not write it down and see what happened. What happened is that every new syscall has shown up with a test suite. So odds are, people will just stick with what we have, leaving it as an "unofficial" rule, as that's easiest, and is what is working well so far (we don't mess with things that are working well...)
The development_process/ document is great, it describes what we normally do, not how we have to do it (a subtle difference).
So I'd recommend changing the MUST to RECOMMENDED, for "all new features need tests" as that's the kernel's "recommended" policy, and I think you will find that other projects also can never meet the "MUST" rule because not everything can be automated or mocked up when software hits external things (hardware, firmware, other programs, etc.)
david-a-wheeler
commented
9 years ago I see the issue. I hate to change it to everything is JUST recommended, because at that point it's not really a badge criteria at all. There are millions of things that are recommended, after all.
Would this work?
Tests are added for new functionality. There MUST be a general policy (formal or not) that when major new functionality is added, tests of that functionality SHOULD be added to an automated test suite. There MUST be evidence that such tests are being added in the most recent major changes to the project. This SHOULD be documented in the instructions for change proposals, but even an informal rule is acceptable as long as the tests are being added in practice. Major functionality would typically be mentioned in the ChangeLog and require at least 1,000 lines of non-comment non-blank lines of code to implement. Perfection is not required, merely evidence that tests are being added in practice.
dankohn
commented
9 years ago I like this, athough 1000 lines seems like a high cutoff for more succinct languages like Ruby or Python.
Dan Kohn mailto:dan@dankohn.com tel:+1-415-233-1000
On Wed, Aug 19, 2015 at 12:02 PM, David A. Wheeler <notifications@github.com
wrote:
I see the issue. I hate to change it to everything is JUST recommended, because at that point it's not really a badge criteria at all. There are millions of things that are recommended, after all.
Would this work?
- Tests are added for new functionality. There MUST be a general policy (formal or not) that when major new functionality is added, tests of that functionality SHOULD be added to an automated test suite. There MUST be evidence that such tests are being added in the most recent major changes to the project. This SHOULD be documented in the instructions for change proposals, but even an informal rule is acceptable as long as the tests are being added in practice. Major functionality would typically be mentioned in the ChangeLog and require at least 1,000 lines of non-comment non-blank lines of code to implement. Perfection is not required, merely evidence that tests are being added in practice.
— Reply to this email directly or view it on GitHub https://github.com/linuxfoundation/cii-best-practices-badge/issues/2#issuecomment-132654067 .
david-a-wheeler
commented
9 years ago On August 19, 2015 1:52:57 PM EDT, Dan Kohn notifications@github.com wrote:
I like this, athough 1000 lines seems like a high cutoff for more succinct languages like Ruby or Python.
Dan Kohn mailto:dan@dankohn.com tel:+1-415-233-1000
On Wed, Aug 19, 2015 at 12:02 PM, David A. Wheeler <notifications@github.com
wrote:
I see the issue. I hate to change it to everything is JUST recommended, because at that point it's not really a badge criteria at all. There are millions of things that are recommended, after all.
Would this work?
- Tests are added for new functionality. There MUST be a general policy (formal or not) that when major new functionality is added, tests of that functionality SHOULD be added to an automated test suite. There MUST be evidence that such tests are being added in the most recent major changes to the project. This SHOULD be documented in the instructions for change proposals, but even an informal rule is acceptable as long as the tests are being added in practice. Major functionality would typically be mentioned in the ChangeLog and require at least 1,000 lines of non-comment non-blank lines of code to implement. Perfection is not required, merely evidence that tests are being added in practice.
— Reply to this email directly or view it on GitHub
https://github.com/linuxfoundation/cii-best-practices-badge/issues/2#issuecomment-132654067 .
Reply to this email directly or view it on GitHub: https://github.com/linuxfoundation/cii-best-practices-badge/issues/2#issuecomment-132719912
We could just omit the sentence entirely. I was trying to be rigorously precise, but with these changes trying to be rigorously precise is probably a waste of time. --- David A.Wheeler
david-a-wheeler
commented
9 years ago I think the raised issues have been addressed. The Linux kernel does in general add tests, as an informal policy, but because of a lack of hardware, not everyone can really test it. The same problem happens elsewhere: people may typically strive to add tests, but various reasons it becomes very different to ALWAYS do it. So the text has been toned down to a level that should be achievable by most projects (including the Linux kernel). For your amusement the current text is below; I'm considering this closed.
New text: There MUST be a general policy (formal or not) that when major new functionality is added, tests of that functionality SHOULD be added to an automated test suite. There MUST be evidence that such tests are being added in the most recent major changes to the project. This SHOULD be documented in the instructions for change proposals, but even an informal rule is acceptable as long as the tests are being added in practice. Major functionality would typically be mentioned in the ChangeLog. Perfection is not required, merely evidence that tests are typically being added in practice.
Greg KH suggests the following in pull request 1, https://github.com/linuxfoundation/cii-best-practices-badge/pull/1 :
"Tests for major feature additions as without some kind of test being present, it's hard to verify if something new even works. This is now the "unofficial" rule for the kernel, as we have been burned by this in the past (feature additions that never even worked)."
I like the idea of adding this to the criteria set, but I think projects need to make it "official" in two senses: (1) their docs on making changes need to require it, and (2) there needs to be some evidence that they're doing it. If it's a super-secret rule, or one not actually used, there's not value. I don't want to add criteria to the "basic" list that few projects meet, though. If this the "unofficial" rule for the kernel, would they be willing to do that?