<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* Areas that require unit tests with each bugfix (Markus/???)<br>
    + <a href="https://wiki.documentfoundation.org/Development/Unit_Tests" rel="noreferrer" target="_blank">https://wiki.documentfoundation.org/Development/Unit_Tests</a><br>
        + updated the page with the areas where tests for fixes are mandatory (Kendy)<br>
        + so far: chart2, writer filters (import + export)<br>
        + do we have more areas where we can have this rule? (Kendy)<br>
            + important to have the maintainer who is checking the code, and the test itself too (Kendy)<br>
            + happened in the past that the test did not fail when the fix was reverted (Kendy)<br></blockquote><div><br></div><div>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.<br><br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
    + would be good to be able to make tests even for bugs that are not fixed yet (Bjoern)<br>
        + happened in the past too, much appreciated, helps fixing (Miklos)<br>
        + do we have a way to mark such a case? (Jan-Marek)<br>
            + not really (Norbert, Miklos)<br>
        + 'make -k stagingcheck' - for tests that do not have a fix yet (Bjoern)<br>
            + isn't it enough to have them in gerrit? (Miklos)<br>
            + can prototype that (Bjoern)<br>
                + that's easy, harder is the infra to show them to the people (Bjoern)<br>
            + might be better to have 'expected failures' tests (Jan-Marek)<br>
                + with easy way to move them to 'expected passes' (Jan-Marek)<br>
        + writing a test needs some knowledge, couldn't we actually mentor the author to do a fix too? (Kendy)<br>
            + can be much harder (Norbert, Bjoern)<br>
        + problem is how to promote the existing tests that don't have the fixes yet (Norbert)<br>
            + more concerned to have some xxx tests run, yyy failed, zzz passed (Norbert)<br>
            + have the special rule could work, would like not to see that some tests may fail<br>
              in the normal build, so that it is not perceived as 'normal that some tests fail (Kendy)<br>
AI:     + will have a look at the CppUnit to implement 'expected failure' (Jan-Marek)<br>
            + Cpp logs are e.g. in workdir/CppunitTest/sal_rtl_math.test.log<br></blockquote><div><br><br></div><div>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.<br><br></div><div>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:<br><br></div><div>void Test::someTestMethod()<br>{<br></div><div>    bool bExceptionThrown = false;<br></div><div>    try<br>    {<br></div><div>        callMethodThatShouldThrow();<br>    }<br></div><div>    catch (ExpectedExceptionType&)<br>    {<br></div><div>         bExceptionThrown = true;<br>    }<br></div><div>    CPPUNIT_ASSERT(bExceptionThrown);<br></div><div>}<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
    + people should look if there has been a -1 (Norbert)<br>
        + -1 is not sticky, it is bound to the version of the patch (Norbert)<br>
        + please look if somebody gave -1 before you are reviewing and pushing, to avoid too<br>
          many people using -2 which is sticky (Norbert)<br>
    + if reverting a patch, please don't forget to let the original author know (Kendy)<br>
        + should we do a git hook to mail when a revert happened? (Norbert)<br>
        + mechanic message maybe could make problems - human interaction probably better? (Bjoern)<br>
        + but letting know is important; usually there is a good reason for a revent (Norbert)<br>
            + it is not personal, it just happens (Norbert)<br>
AI:         + can have a look (Norbert)<br>
                + mail to the author + committer + the person who reverted (Bjoern)<br>
        + best to get more reviewers in gerrit, so that reverts are seldom (Bjoern)<br>
    + how to handle when something is pushed to these areas anyway?<br>
        + the same rules as when breaking the build (Miklos)<br>
            + when the author is responsive, and working on the test, let it in (Miklos)<br>
            + when not, revert (Miklos)<br>
            + timing is up to the maintainer (Miklos, Norbert)<br>
            + it is always a trade-off (Bjoern)<br>
                + best to talk to the maintainer, not to overrule him/her (Norbert)<br>
            + important to talk to people, don't push without talking when overruling (Eike)<br>
<br></blockquote></div><br></div></div>