[RFC] clar: known breakage - #4500
Open
pks-t wants to merge 7 commits into
Open
Conversation
Member
Author
|
Heh. Tests fail on Windows because the test in online::badssl is not actually broken on Windows, but on Linux platforms only. So it already pays off :P |
Member
|
I understand the reasoning here. I'm still not fond of the idea of merging this feature for upstream Clar but I think it makes sense for the libgit2 project. |
Member
Author
|
Fair enough, I guess. Thanks for chiming in on that, @vmg! |
Contributor
|
I'm 👍 with this. I'm not sure I quite like the name too, but my (preferred) solution would be to have macros like |
This was referenced Mar 19, 2018
Setting a new error requires quite a lot of code. As we want to re-use this code later on, refactor it into its own function.
Printing out errors can be based on the type of error status. While there is currently no status besides test failures which is to be printed, we will introduce a failure type "Unbroken" soon. As such, add the error status to the structure so that we're able to distinguish errors and print them out differently.
Currently, we have no way to record known breakages in test suites. If a projects knows of a certain way in which functionality is broken but cannot yet fix it, it may want to record this fact. This provides some benefits: - breakage is recorded and easily discoverable, so that committers may pick out known breakages to pick up work - it becomes possible to submit non-working tests first demonstrating breakage and later on submit a fix, switching over to a normal assert - an error can be thrown as soon as the test is _not_ broken anymore, clearly highlighting when another change fixes breakage This commit introduces a new state `CL_TEST_BROKEN` to mark a test as broken. It also introduces a new macro `cl_break` which calls into the new function `clar__broken`, which can be used to mark a test case as broken.
Next to explicitly setting a breakage inside of a test with `cl_break`, the most common scenario will most likely be to have known breakage on a condition. This patch introduces a new set of macros `cl_must_break` and `cl_check_break`. These macros cause a test to either be set to "broken" if the assertion holds or, if it does not, to "failure". Like this, the test can notify on both conditions: - the system under test is still broken - the system under test has been fixed and is not broken anymore, resulting in a failure Note that a failure is the only reasonable thing to do. The authors of the test suite certainly want to be notified as soon as known breakages get fixed such that they can adjust tests and potentially e.g. remove TODO items or bug tickets associated with the previous breakage.
pks-t
force-pushed
the
pks/clar-known-breakage
branch
from
April 6, 2018 13:12
786b206 to
0216419
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces the concept of known breakages to clar. A known breakage is a bug which exists in our code base which we are currently unable to fix, but which is known to exist. While we have some such tests in our code base (e.g. online::badssl::old_cipher), we just
cl_skip()those tests. This has two disadvantages:With known breakages, we can fix this both problems. This PR introduces two new test statuses: "broken" ('B') for a known-broken test and "unbroken" ('U'). Tests which are known broken have a status code of 'B' such that they are easily discoverable, but those tests will not cause an error. In case the breakage goes away, though, the test will return an "unbroken" status and create an error. So first, we know about all broken tests, addressing the first disadvantage, and second we will know when the test gets unbroken, addressing the second disadvantage.
That being said, this was not accepted at clar upstream, based on the reasoning that a breakage should just be fixed (see clar-test/clar#77). I don't really think that works for open source projects (and probably also not for software projects in general), as contributors might just not have enough free time to fix the issue in case it is something deeper. But there is still value in documenting the issue in code to have a technically verifiable documentation of it. Anyway, cc'ing @vmg such that he can correct me if I've misrepresented his reasoning.
The above is also the reason why I made this PR an RFC, only. Furthermore, I think the naming is a bit hard to get right here, especially to macros like
clar__break,clar_libgit2_brokenetc. I'd welcome better proposals.