output-distance: treat non-existent files as empty string

output-distance: treat non-existent files as empty string

This avoids callings strip() on None

[hahnjo, 2020-02-28 17:34:20]

LGTM

[Dan Eble, 2020-02-28 23:31:17]

This does not look good.  I expect this change to break the feature I added in
0d72930e579a5784ecb26da2f9880d8c9da05e71 (Issue 5635). Well, a call to strip
(None) is possibly evidence that someone already broke it, but let's at least
avoid making it worse.

I don't believe you really want a validator to pretend that a missing file
exists.

[Dan Eble, 2020-02-28 23:33:32]

On 2020/02/28 23:31:17, Dan Eble wrote:
> This does not look good.  I expect this change to break the feature I added in
> 0d72930e579a5784ecb26da2f9880d8c9da05e71 (Issue 5635). Well, a call to strip
> (None) is possibly evidence that someone already broke it, but let's at least
> avoid making it worse.
> 
> I don't believe you really want a validator to pretend that a missing file
> exists.

[now with the "send mail" option checked]

[hanwenn, 2020-02-29 12:49:11]

On 2020/02/28 23:33:32, Dan Eble wrote:
> On 2020/02/28 23:31:17, Dan Eble wrote:
> > This does not look good.  I expect this change to break the feature I added
in
> > 0d72930e579a5784ecb26da2f9880d8c9da05e71 (Issue 5635). Well, a call to strip
> > (None) is possibly evidence that someone already broke it, but let's at
least
> > avoid making it worse.
> > 
> > I don't believe you really want a validator to pretend that a missing file
> > exists.
> 
> [now with the "send mail" option checked]

done.

[hanwenn, 2020-02-29 12:51:30]

Dan comment

[Dan Eble, 2020-02-29 15:51:44]

https://codereview.appspot.com/583590043/diff/571790043/scripts/build/output-...
File scripts/build/output-distance.py (right):

https://codereview.appspot.com/583590043/diff/571790043/scripts/build/output-...
scripts/build/output-distance.py:531: if self.contents[0] is None !=
self.contents[1] is None:
This symmetric condition no longer matches the comment immediately below.  It
seems that you would also avoid showing the diff when the baseline version has a
file that is missing in the new version.  I have mixed opinions about that.

In the case of an intentional removal of a test case, I probably wouldn't mind
seeing a striped cell on the right-hand side of the page, but I think that might
be inconsistent with the way other types of files are still being handled in
this case.  IIRC, removed test cases currently do not appear in the results
except as a count, and I slightly prefer keeping it that way.

In the case of an error causing an expected text file to be missing, I think I
would prefer output that is harder to overlook.  The full diff output will
probably qualify as harder to overlook, though it's not the only option.  Having
the script fail on None.strip(), though not the best option, is still better
than treating this as discreetly as the intentional removal of a test case.

My first approach would probably have been to leave this earlier stuff alone and
address the issue below (untested):

-   cont1 = self.contents[1].strip();
+   cont1 = self.contents[1].strip() if self.contents[1] else ''

These changes deserve some systematic manual testing; I hope you're covering
that.  If this script fails to publish differences when it should, nobody else
involved in the countdown process is going to notice.

[hanwenn, 2020-03-05 10:29:35]

reword comment

[hanwenn, 2020-03-05 10:33:57]

https://codereview.appspot.com/583590043/diff/571790043/scripts/build/output-...
File scripts/build/output-distance.py (right):

https://codereview.appspot.com/583590043/diff/571790043/scripts/build/output-...
scripts/build/output-distance.py:531: if self.contents[0] is None !=
self.contents[1] is None:
On 2020/02/29 15:51:43, Dan Eble wrote:
> This symmetric condition no longer matches the comment immediately below.  It
> seems that you would also avoid showing the diff when the baseline version has
a
> file that is missing in the new version.  I have mixed opinions about that.

I adjusted the comment.

> the script fail on None.strip(), though not the best option, is still better
> than treating this as discreetly as the intentional removal of a test case.

I disagree. Having the script crash prevents us from forming a complete
impression of what a change does, because the crash can hide other regression
failures.
 
> My first approach would probably have been to leave this earlier stuff alone
and
> address the issue below (untested):
> 
> -   cont1 = self.contents[1].strip();
> +   cont1 = self.contents[1].strip() if self.contents[1] else ''

this change is to address the failures occurring in 
https://sourceforge.net/p/testlilyissues/issues/5785/

caused by contents[0] being None.

> These changes deserve some systematic manual testing; I hope you're covering
> that.  If this script fails to publish differences when it should, nobody else
> involved in the countdown process is going to notice.

this is why I am extending the if case to be symmetric. We already know that the
addition of files is handled correctly. Making a minimal change here avoids some
QA.

[Dan Eble, 2020-03-06 01:03:49]

On 2020/03/05 10:33:57, hanwenn wrote:
> > the script fail on None.strip(), though not the best option, is still better
> > than treating this as discreetly as the intentional removal of a test case.
> 
> I disagree. Having the script crash prevents us from forming a complete
> impression of what a change does, because the crash can hide other regression
> failures.

I want to make sure that it is clear that I did not say that having the script
crash was the best option, just better than acting like there is no difference. 
Listing no diff when an expected text file is missing would prevent us from
forming a complete impression of what a change does; I'm concerned that you seem
unconcerned about that.
 
> > These changes deserve some systematic manual testing; I hope you're covering
> > that.  If this script fails to publish differences when it should, nobody
else
> > involved in the countdown process is going to notice.
> 
> this is why I am extending the if case to be symmetric. We already know that
the
> addition of files is handled correctly. Making a minimal change here avoids
some
> QA.

It's not completely clear to me, but what I infer from this is that the extent
of your testing has been to run make check and observe that it no longer
crashes.  Is that right, or have you looked closely enough to describe how the
two cases I previously mentioned are presented now?  My biggest concern is that
table of differences might have no row for the missing file.  I don't think it's
safe to assume that the calling code handles removal like it handles addition;
my reason for believing this is that I implemented the handling of added test
cases, and I did not aim to handle removed test cases when I did it.

[hanwenn, 2020-03-06 16:28:01]

On 2020/03/06 01:03:49, Dan Eble wrote:
> On 2020/03/05 10:33:57, hanwenn wrote:
> > > the script fail on None.strip(), though not the best option, is still
better
> > > than treating this as discreetly as the intentional removal of a test
case.
> > 
> > I disagree. Having the script crash prevents us from forming a complete
> > impression of what a change does, because the crash can hide other
regression
> > failures.
> 
> I want to make sure that it is clear that I did not say that having the script
> crash was the best option, just better than acting like there is no
difference. 
> Listing no diff when an expected text file is missing would prevent us from
> forming a complete impression of what a change does; I'm concerned that you
seem
> unconcerned about that.
>  
> > > These changes deserve some systematic manual testing; I hope you're
covering
> > > that.  If this script fails to publish differences when it should, nobody
> else
> > > involved in the countdown process is going to notice.
> > 
> > this is why I am extending the if case to be symmetric. We already know that
> the
> > addition of files is handled correctly. Making a minimal change here avoids
> some
> > QA.
> 
> It's not completely clear to me, but what I infer from this is that the extent
> of your testing has been to run make check and observe that it no longer
> crashes.  Is that right, or have you looked closely enough to describe how the
> two cases I previously mentioned are presented now?  My biggest concern is
that
> table of differences might have no row for the missing file.  I don't think
it's
> safe to assume that the calling code handles removal like it handles addition;
> my reason for believing this is that I implemented the handling of added test
> cases, and I did not aim to handle removed test cases when I did it.

I spent some time today to look into this, and you were (of course) right. 

It turned out to be a big change; it's over here  

 
https://codereview.appspot.com/581770043/diff/561510043/scripts/build/output-...

it leaves striped cells for removed files. I don't think it's feasible to do
much better, or we'd have to extend output-distance to infer what kind of files
are to be expected. That will likely be brittle logic.

I am also both proud and annoyed at myself 13 years ago, because I added a test
to output-distance (see the --test flag), but then didn't integrate it into the
build, and it's been broken since July 2007.

If you want to see cleanup separate from logic changes, see 

  https://github.com/hanwen/lilypond/commits/output-distance-test

[hanwenn, 2020-03-06 16:47:49]

(https://github.com/hanwen/lilypond/pull/2 as a PR)

[Dan Eble, 2020-03-07 01:02:48]

On 2020/03/06 16:28:01, hanwenn wrote:
>  
>
https://codereview.appspot.com/581770043/diff/561510043/scripts/build/output-...
> 
> it leaves striped cells for removed files. I don't think it's feasible to do
> much better, or we'd have to extend output-distance to infer what kind of
files
> are to be expected. That will likely be brittle logic.

I haven't looked at the code yet, but I think this treatment is acceptable.
-- 
Dan

[hanwenn, 2020-03-07 09:33:27]

abandoning for 5824