Re: Advancing to Patch::review

[Michael Käppler]

Am 03.12.2020 um 12:06 schrieb James:
> Hello,
>
> On 02/12/2020 20:57, Michael Käppler wrote:
> > Am 02.12.2020 um 18:16 schrieb Jonas Hahnfeld:
> >
> > [snip]
> > > Circling back to my original proposal:
> > > > My gut feeling is that this should be somebody else than the MR author
> > > Do I interpret your actions that you disagree with this? To elaborate a
> > > bit, this tries to keep the pleasant effect that somebody else at least
> > > opens your MR and nobody is tempted to change labels because "I'm sure
> > > this will pass testing".
> > Well, I agree that it is better to have at least four eyes look at the
> > test results,
> > but since you wrote "You're not sold on this" I thought it would be okay
> > for you if I check the tests myself.  I did not include visual
> > screenshots, sorry for that.
> >
> > Do you have any other 'actions' in mind? I assume that checking the
> > results
> > for !533 and setting it to 'review' was ok?
> >
> > If we decide to have the policy "Do not set your own MRs to 'review'",
> > I'm also
> > fine with that. OTOH, I would also trust the developers that they do not
> > set MRs to 'review' without looking at the tests.
> >
> > I'm not sure, however, if it does work well to distribute the job of
> > setting
> > patches from 'new' to 'review', because the frequency of somebody
> > 'passing by'
> > can vary to a great extent and sometimes shared responsibility is
> > noone's responsibility :)
>
> Yes the classic 'Bystander Effect' problem.
>
> > James, what is your opinion on that? Would you still be willing to do
> > this job?
>
>
> Testing Patches was always orthogonal to managing the countdown (which
> was the original point of the 'Patch Meister' role).
>
> I only fell into the testing patches because at the time we had
> limited resources (hardware was not what it is now) and only a few
> really active LP dev team members. We were missing a lot of fly-by
> patches, and some patches were being left to 'die' because no one was
> reviewing them. I was already doing doc patches and didn't mind
> stepping in to help the LP team with the testing. The countdown was
> being done by another volunteer at the time and I only took that on
> because he decided he didn't want to do it any more and left. I was
> worried that we'd lose patches or the current devs would not want to
> be bothered with the managing of all these disparate diff files and
> git-formatted patches flying about in emails and in reitveld.
>
> So I ended up with two roles. Testing and Patch Meister.
Great that you did this reliably for a pretty long time now! I think
that was (and is) of great value for the LilyPond community.
> 'Meistering' patches is trivial because I can do it in a few minutes
> and on a regular schedule (more or less) and this seems to be working
> OK - I have no 'skin' in the game as I am not a developer and I hope I
> bring a sense of fairness to the countdown for patches that are more
> contentious than others.
>
> As for Testing?
>
> I am more than happy to 'let go' of the Patch testing frankly, I don't
> mind doing it, but it does seem silly that this should not be fully
> automated now.
>
> Also I still have to 'look' at the MR for the countdown process, and
> if somehow we could post the 'URL link' for the diff in the MR thread
> automatically (rather than go and hunt around for it by clicking icons
> etc) then that would remove a lot of friction but I think that is not
> easy/possible to do.
I would propose that the dev that opens a new MR is responsible for
pasting the link and a screenshot. Then you could have a look at
MRs with Patch::new on a regular schedule and if you agree that the reg
tests look good set it to Patch::review. That way we would have
at least four eyes looking at the tests.
> I also realise that reg tests are critical to what LP stands for and
> we do need that human eye on them, but even so... I think we should
> see how it goes (i.e. fully automated and any other dev can check the
> diff from now) and trust that the devs will not abuse the system and
> actually check something than just move on the patch. But as far as I
> understand it the make-check passing will put the review label on the
> MR right?
No, that step remains manually, if I'm not mistaken.
IIUC, a patch that fails 'make check' ('fails' in the sense of 'errors
out') would have failed 'make test' already.
As I understood the (old) process, setting a patch to 'Review' should
not only require a 'passing' 'make check', but also
a visual inspection of the results.
So setting a MR to 'Review' automatically after passing 'make check'
would be a policy change IMHO.

> Perhaps we need to add a 'step' before we release a new version to the
> world, by running a full reg test suite and posting it somewhere - I
> seem to recall that we used to do that in the past, I think Phil had
> some hacked script he did on his own website that showed diffs between
> various significant versions. Is something we could do - I don't know
> enough about what we can and cannot do in Gitlab that we could 'post'
> somewhere or even view in the repo (even if it was binary output like
> a PDF)?
What do you mean with "full reg test suite"? Additional examples that
are not in the normal reg test suite?
I think comparisons between several versions are of limited usefulness,
because it can be difficult to see if
changes are intended, not intended, or even occuring at random.
In my opinion the current test suite has conceptual problems that go
beyond that discussion.
E.g. that we don't compare actual output, but grob positions and
extents, AFAIK.
That lead to problems in Harm's MR:
https://gitlab.com/lilypond/lilypond/-/merge_requests/497
Also issues like https://gitlab.com/lilypond/lilypond/-/issues/720 that
affect specific backends cannot be
tested (or it would require hacks)

Michael

> James
>
>
>
> > Cheers,
> > Michael