[GNC-dev] merging failing tests

[John Ralls]

Carsten:

Distilling this to the core, “Why will we not merge a PR with failing tests?”

Simple: If you have a test that exposes a bug, then fixing the bug is part of the job. The branch isn’t ready to merge until it’s fixed. That’s because the point of unit tests is to tell developers when they’ve broken something and the only way that that can work is for all tests to always pass. If there are failed tests then the TravisCI email about failing tests on every push becomes routine and the exercise becomes pointless.

It’s a bit disingenuous of you to cite PR391 when the comments about failing tests and not writing proper SRFI-64 tests were on PR404. Your latest push to PR391, which I hadn’t had time to review until now, is much better. It looks from the output on TravisCI that there’s one actual problem with gnc-html, that it outputs <string> </string> when it should output <boolean>#f</boolean>, and several with your expected html. 

PR404 needs at least the same makeover. If you can figure out how to break up tests 6-9 into tests that are smaller than whole documents they’d be a lot more useful and easier to diagnose problems. Trying to find the discrepancy between two 70-line strings when only 32 lines are visible on a TravisCI web page will be an exercise in futility.

Regards,
John Ralls

> On Sep 5, 2018, at 2:12 PM, Carsten Rinke <carsten.rinke at gmx.de> wrote:
> 
> Hi John,
> 
> I transfer this thread to this channel because I think this is not PR specific but more a general issue.
> 
> Let me start by saying that I do not want to "fight to get it merged". For me it is an interesting point to exchange views upon.
> 
> It is specifically about "We're not merging failing tests".
> 
> Example: PR#391
> I have tried to follow your advice how to write SRFI-64 as good as I can at this point of time (I'm sure you will still find room for improvement, and that is fully ok).
> 
> In the test result from travis you can now see which tests are failing, and why, and I even mentioned the bug reports that I opened for it.
> 
> That the tests are not merged straight away is expected, no discussion about it, especially as long as the indicated bugs are not finally accepted as bugs.
> 
> But assume we agree that some bugs are really bugs, meaning, the test is correct, and it is correct that it is failing because the code is wrong:
> Why not merging a failing test? Why waiting for the code fix?
> 
> Just to repeat: Please take these as open questions, I am not suggesting an answer here.
> 
> BTW:
> I think I can propose fixes for most of the indicated bugs in PR#391, but I was planning to forward them as separate PRs. Good that you mention that I should place them into the existing PR.
> 
> /Carsten
> 
> 
> Am 05.09.2018 um 01:29 schrieb John Ralls:
>> 
>> Since you haven't followed my instructions about how to write a SRFI-64 test it's impossible to tell from the output what is the problem... including knowing whether there's actually a bug in the gnc-html code or if it's in your tests. Yes, that's a defect in most of the Scheme test code, but fixing it was one of the main goals of adopting a proper unit test framework.
>> 
>> If your tests did find bugs then the PR needs to include fixes as separate commits. We're not merging failing tests and we're not merging non-tests. Consider that both of the failing tests in this PR seem not to matter: All of the existing report tests pass and the line charts work in use. If an important jqplot module isn't present or if the line chart plotting code was actually defective on Arch Linux that wouldn't be the case.
>> 
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub <https://github.com/Gnucash/gnucash/pull/404#issuecomment-418550184>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AmB1_ItHUF6N8W7RKcowHMxz1pKkW_5sks5uXwzOgaJpZM4WM2Ls>.
>> 
> 
> _______________________________________________
> gnucash-devel mailing list
> gnucash-devel at gnucash.org
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel