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