Re: [PATCH v2] maintenance tests: fix systemd v2.34.0-rc* test regression

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 11, 2021, 17:45 UTC
Message-ID: <xmqqsfw2mvqn.fsf@gitster.g>
In-Reply-To: <211110.86ilx0gdx8.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 18 quoted lines

> On Wed, Nov 10 2021, Johannes Schindelin wrote:
>
>> Hi Ævar,
>>
>> On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Fix tests added in b681b191f92 (maintenance: add support for systemd
>>> timers on Linux, 2021-09-04) to run successfully on systems where
>>> systemd-analyze is installed, but on which there's a discrepancy
>>> between a FILE argument of "/lib/systemd/system/basic.target" and
>>> "systemd/user/git-maintenance@.service" succeeding.
>>
>> Could you try to rephrase `there's a discrepancy between a FILE argument
>> of "/lib/systemd/system/basic.target" and
>> "systemd/user/git-maintenance@.service" succeeding` more clearly?
>
> Sure. Briefly to test if X works our prereq should test if X can work,
> not if Y can work. Will try to update it.

Thanks.

Show 11 quoted lines

>>>  test_systemd_analyze_verify () {
>>> -	if test_have_prereq SYSTEMD_ANALYZE
>>> -	then
>>> -		systemd-analyze verify "$@"
>>> -	fi
>>> +	# Ignoring any errors from systemd-analyze is intentional
>>> +	systemd-analyze verify "$@" >systemd.out 2>systemd.err;
>>
>> The semicolon is superfluous.
>
> We use it in other places as shorthand for "I mean to not have && here".

That is news to me (not the presense of unnecessary semicolons; I do not think we have "when you want to make it clear that you deliberately did not write &&, write ;" in any of the guidelines, and I am not sure if such a guideline is a good idea).

Show 5 quoted lines

>> It is a bit sad that we're now doing unnecessary work when
>> `systemd-analyze` does not even exist.
>
> It's a chicken & egg problem. How do you figure out if you're able to
> run the command & get the output you expect without doing that?

I read "It is a bit sad" to imply "... but it probalby is the best we can do, and more importantly, it is not worse than the problem it is solving".

Show 18 quoted lines

> +		-e '/git-maintenance@i*\.service:/x' \
>>
>> Lines containing `git-maintenance@.service:` (or the same pattern with an
>> arbitrary number of `i`s after the `@` character???) are exchanged with
>> hold space.
>>
>> That does not look right.
>
> It'll emit @.service or @i.service. I have no idea why, yeah the regex
> is overly generous, but it doesn't hurt anything in this case (see
> below)>
>
>> Since this is a `sed -n` call, we would need an explicit `p` command to
>> print anything. Therefore, the current code is a pretty expensive
>> equivalent to calling `true >&6`: it succeeds, and it does not print
>> anything.
>
> Yes, like the buggy "if the prereq succeeds" code it replaced.

If both are buggy, then there is no point replacing old with new. Let's have a non-buggy improved new and replace buggy old with it ;-)

Show 9 quoted lines

> Because I really don't know. Is it broken on literally one machine in
> the world that I happened to have tested on, or more generally on the
> sorts of OS version/whatever that has that systemd? No idea.
>
> But the worst we'll do here is not run a test on some obscure setup.
>
> Since the value of having the test is there if we just run it on some
> setups that's fine. We should lean towards over-suppressing it to not
> have "make test" in v2.34.0 fail on some platforms.

Hmph, if we _know_ (or _think_) the platforms that would fail a particular test is rare enough *and* if we know we _don't_ understand why the test fails on them, wouldn't we want to know the extent of the damage first by not suppressing the test and let it break on these platforms that may help us after the release to understand the breakage and deal with it? The resolution may end up to suppress the test after all, but with the approach we can do so knowing why we are doing so, no?

This is different from OpenSSH 8.7 thing that we _know_ why the test breaks, and we _know_ that there will be tons of instances with that broken version. Suppressing the test for them does make sense.

Thanks.