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-distance.py 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 https://codereview.appspot.com/583590043/
