[GNC-dev] merging failing tests

Christopher Lam christopher.lck at gmail.com
Wed Sep 5 18:28:41 EDT 2018


As an untrained hacker, I think the issue is whether these "bugs" are 
really worth fixing at all.

I can imagine the history of the dark ages; and html was being refined, 
and previously the reports were outputting plaintext or directly to 
screen via X calls; the hackers decide to start outputting html via a 
new upcoming framework called gtkhtml, with fancy tables, italics, font 
sizes... GTKhtml comes with an over-engineered api to accept x/y 
coordinates, font families, headers, pre-CSS style-sheets, paper size. 
The hacker succeeds in outputting reports and uses the api calls to 
render a nice report, helpfully reusing gtkhtml terminology.

Time has evolved, and gtkhtml is now obsolete. The api calls must be 
changed to output html. The next hacker tries to fix the minimum code to 
remove gtkhtml code, but every time a change happens, some reports break 
or dataloss occurs. Therefore they fix the minimum code to move away 
from gtkhtml and leave large swathes of dead old code around. "If it's 
not broke, don't fix it!"

I think that completing the internal framework to successfully implement 
html api code (as you have identified is incomplete) is not time well 
spent. None of the reports will generate html without headers, for 
instance. So, the code for testing headers should be ripped out, rather 
than implemented. And code should be ripped out safely, ensuring no 
tests fail, no dataloss occurs, all reports pass (including the existing 
multicolumn report) and all potential users (including those not 
subscribed to userlist or devel) are unaffected, and no subsequent 
hacker will have to fix.

There is still a need for more hackers, so, I'm sure your efforts are 
appreciated, but it's already a major headache to try understand the 
previous hackers and we don't want the situation to get worse.

I think this helps? I'd love to add more features too but I'm 
constraining myself too because every new code snippet, framework, or 
boolean test is another headache for future maintainers.

On 06/09/18 05:12, Carsten Rinke 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

More information about the gnucash-devel mailing list