Skip to content

[RFC] clar: known breakage - #4500

Open
pks-t wants to merge 7 commits into
libgit2:mainfrom
pks-t:pks/clar-known-breakage
Open

[RFC] clar: known breakage#4500
pks-t wants to merge 7 commits into
libgit2:mainfrom
pks-t:pks/clar-known-breakage

Conversation

@pks-t

@pks-t pks-t commented Jan 26, 2018

Copy link
Copy Markdown
Member

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:

  • a test skipped because of known breakage cannot be distinguished from a test that was skipped e.g. because certain code was not compiled into our library
  • in case the bug gets fixed, we will not get any feedback from our tests, as that test is still going to be just skipped over

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_broken etc. I'd welcome better proposals.

@pks-t

pks-t commented Jan 26, 2018

Copy link
Copy Markdown
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

@vmg

vmg commented Jan 26, 2018

Copy link
Copy Markdown
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.

@pks-t

pks-t commented Feb 1, 2018

Copy link
Copy Markdown
Member Author

Fair enough, I guess. Thanks for chiming in on that, @vmg!

@tiennou

tiennou commented Feb 7, 2018

Copy link
Copy Markdown
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 cl_(un)?expected_(success|failure) (which is far-reaching), so maybe something like cl_should_(fail|pass) ?

pks-t added 7 commits April 6, 2018 14:10
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
pks-t force-pushed the pks/clar-known-breakage branch from 786b206 to 0216419 Compare April 6, 2018 13:12
Base automatically changed from master to main January 7, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants