Re: [Libreoffice-qa] Minutes of ESC call: 2015-07-30

> * Areas that require unit tests with each bugfix (Markus/???)
>     + https://wiki.documentfoundation.org/Development/Unit_Tests
>         + updated the page with the areas where tests for fixes are
> mandatory (Kendy)
>         + so far: chart2, writer filters (import + export)
>         + do we have more areas where we can have this rule? (Kendy)
>             + important to have the maintainer who is checking the code,
> and the test itself too (Kendy)
>             + happened in the past that the test did not fail when the fix
> was reverted (Kendy)

So just to make it clear as it seems that I gave the wrong impression. I
don't insist on all chart2 commits having a test (it would be nice if
everyone would write a test), however if I manage to review a patch and ask
for a test to a chart2 patch I expect that the patch is not pushed without
the test. In general I will ask for a test in all chart2 patches where it
makes sense. Of course I would be even more happy if everyone would just
write the test and I would only need to make sure that the test passes and
fails without the fix.


    + would be good to be able to make tests even for bugs that are not
> fixed yet (Bjoern)
>         + happened in the past too, much appreciated, helps fixing (Miklos)
>         + do we have a way to mark such a case? (Jan-Marek)
>             + not really (Norbert, Miklos)
>         + 'make -k stagingcheck' - for tests that do not have a fix yet
> (Bjoern)
>             + isn't it enough to have them in gerrit? (Miklos)
>             + can prototype that (Bjoern)
>                 + that's easy, harder is the infra to show them to the
> people (Bjoern)
>             + might be better to have 'expected failures' tests (Jan-Marek)
>                 + with easy way to move them to 'expected passes'
> (Jan-Marek)
>         + writing a test needs some knowledge, couldn't we actually mentor
> the author to do a fix too? (Kendy)
>             + can be much harder (Norbert, Bjoern)
>         + problem is how to promote the existing tests that don't have the
> fixes yet (Norbert)
>             + more concerned to have some xxx tests run, yyy failed, zzz
> passed (Norbert)
>             + have the special rule could work, would like not to see that
> some tests may fail
>               in the normal build, so that it is not perceived as 'normal
> that some tests fail (Kendy)
> AI:     + will have a look at the CppUnit to implement 'expected failure'
> (Jan-Marek)
>             + Cpp logs are e.g. in
> workdir/CppunitTest/sal_rtl_math.test.log


That is already possible with cppunit. Instead of using CPPUNIT_TEST use
CPPUNIT_TEST_FAIL which tells cppunit that the test is expected to fail
with a cppunit exception being thrown (it is extensively used in the
cppunit internal unit tests). The test will start to fail when none of the
asserts fail anymore. Keep in mind that it might be dangerous to use this
with more than one assertion as an unexpected one might fail.

Additionally there is CPPUNIT_TEST_EXCEPTION which expects as second
parameter the expected exception. So this is a replacement for the
following pattern that can sometimes be found in our code:

void Test::someTestMethod()
{
    bool bExceptionThrown = false;
    try
    {
        callMethodThatShouldThrow();
    }
    catch (ExpectedExceptionType&)
    {
         bExceptionThrown = true;
    }
    CPPUNIT_ASSERT(bExceptionThrown);
}


>     + people should look if there has been a -1 (Norbert)
>         + -1 is not sticky, it is bound to the version of the patch
> (Norbert)
>         + please look if somebody gave -1 before you are reviewing and
> pushing, to avoid too
>           many people using -2 which is sticky (Norbert)
>     + if reverting a patch, please don't forget to let the original author
> know (Kendy)
>         + should we do a git hook to mail when a revert happened? (Norbert)
>         + mechanic message maybe could make problems - human interaction
> probably better? (Bjoern)
>         + but letting know is important; usually there is a good reason
> for a revent (Norbert)
>             + it is not personal, it just happens (Norbert)
> AI:         + can have a look (Norbert)
>                 + mail to the author + committer + the person who reverted
> (Bjoern)
>         + best to get more reviewers in gerrit, so that reverts are seldom
> (Bjoern)
>     + how to handle when something is pushed to these areas anyway?
>         + the same rules as when breaking the build (Miklos)
>             + when the author is responsive, and working on the test, let
> it in (Miklos)
>             + when not, revert (Miklos)
>             + timing is up to the maintainer (Miklos, Norbert)
>             + it is always a trade-off (Bjoern)
>                 + best to talk to the maintainer, not to overrule him/her
> (Norbert)
>             + important to talk to people, don't push without talking when
> overruling (Eike)