Issue 583590043: output-distance: treat non-existent files as empty string (Closed)
|
Created:
5 years ago by hanwenn Modified:
5 years ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
Descriptionoutput-distance: treat non-existent files as empty string
This avoids callings strip() on None
Patch Set 1 #Patch Set 2 : Dan comment #
Total comments: 2
Patch Set 3 : reword comment #MessagesTotal messages: 13
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.
Sign in to reply to this message.
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]
Sign in to reply to this message.
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.
Sign in to reply to this message.
Dan comment
Sign in to reply to this message.
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.
Sign in to reply to this message.
reword comment
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
Sign in to reply to this message.
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
Sign in to reply to this message.
|
