[GNC-dev] merging failing tests

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[GNC-dev] merging failing tests

Carsten Rinke
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
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] merging failing tests

Christopher Lam
Hi

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
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] merging failing tests

John Ralls-2
In reply to this post by Carsten Rinke
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 <[hidden email]> 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
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] merging failing tests

Geert Janssens-4
In reply to this post by Carsten Rinke
Op woensdag 5 september 2018 23:12:31 CEST schreef Carsten Rinke:
> Why not merging a failing test? Why waiting for the code fix?

Aside from John's explanation I would add this as a reason:
Maint and master are integration branches. Our intention is to keep them in a
releasable state all the time. That is at any point in time we should be able
to take their current state and release it.

I wrote "intention" because in practice new commits can always introduce bugs
that are not immediately detected. Especially as our test coverage is far from
complete. So we are not there yet. But the intention is there and a branch
with failing tests is not in a releasable state. So failing tests are ok on
feature branches but they should be fixed (or dropped/disabled if irrelevant
or not critical) before they can be merged.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] merging failing tests

Geert Janssens-4
In reply to this post by Christopher Lam
Op donderdag 6 september 2018 00:28:41 CEST schreef Christopher Lam:
<snip>

> 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.
Yet they should be able to for a properly validating multi-column report. This
is IMO a valid bug in the current state [1]. So either header omission should
be implemented or the concept of a multi column report needs to be reworked.

To zoom in a bit on this issue: there is more to do than just omitting the
headers. The multi-column report only needs the contents of the <body> tag and
optionally any header elements (css,  stylesheet, whatever) that were in the
original header and needed for the original report to function properly. So it
may be easier to write code in the multi-column report to extract this data
rather than make all reports deal with optional headers but with separated
style info.

Other than that I'm all for seriously reducing the complexity of the html and
stylesheet generating code.

Geert

[1] Which I did report years ago: https://bugs.gnucash.org/show_bug.cgi?
id=706266


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] merging failing tests

Carsten Rinke
Thanks for all your comments.

@John, Geert:
Thanks for bringing the expectations to the point that unit tests as
such and especially on master/maint always must pass.
I also get the point that further refinement of the test results by
utilitzing the verdicts "xpass" and "xfail" is not desired. I have taken
these out now (in #391, #404 to come).

Regarding: "disingenuous" - I needed to look it up.
Seems to be a very strong word of disappointment. My apologies for
provoking this reaction. That was absolutely not my intention. Using
#391 is just more appropriate as it already includes changes which I did
not have a chance yet to put them to #404. That is why talking about
#391 - that has the same problem as #404 - seemed more meaningful to me.

In General:
 From the pure fact that code is not used it is very hard for me to
judge if the code is not needed.
I can only see that a feature is already there, and conclude from this,
that at some point it was agreed to be there. If it is not working, it
should be fixed.
And of course I see the point that the effort to fix bugs that have not
been complaint about should be balanced. In this special case the items
can be fixed in reasonable short time.
By that is is okay for me, if these findings will never end up in
master/maint. Getting to know the code and getting to know your
expectations about proper testing (and getting some more historical
lessons about GnuCash) is already a great gain.

Carsten
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel