| Message ID | patch-v2-1.1-44f0cafa16e-20211110T035103Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] maintenance tests: fix systemd v2.34.0-rc* test regression | expand |
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? Also, commit messages in git.git should not assume that every reader has a profound knowledge of `systemd`. The commit message needs to do a better job at explaining what is broken in the first place. The CI runs pass, after all, so it is unclear that there is anything broken that would need fixing to begin with. > There was an attempt to work around previous breakage in these tests > in 670e5973992 (maintenance: fix test t7900-maintenance.sh, > 2021-09-27), as noted in my [1] that commit is wrong about its > assumption that we can use "/lib/systemd/system/basic.target" as a > canary.argument. > > To fix this let's adjust this test to test what it really should be > testing: If we've got systemd-analyze reporting anything useful, we > should use it to check the syntax of our just-generated > "systemd/user/git-maintenance@.service" file. What is "anything useful" here? And how can we use the output of `systemd-analyze` to check the syntax of the generated `.service` file? This is all very unclear. > Even on systems where this previously succeeded we weren't effectively > doing that, because "systemd-analyze" will pass various syntax errors > by and exit with a status code of 0, e.g. if the "[Unit]" section is > replaced with a nonsensical "[HlaghUnfUnf]" section. > > To do that ignore whatever exit code we get from "systemd-analyze > verify", and filter its stderr output to extract the sorts of lines it > emits on note syntax warnings and errors. We need to filter out > "Failed to load", which would be emitted e.g. on the > gcc135.fsffrance.org test box[1]. The domain name is not as interesting as the verbatim error message would be. > We also need to pipe this output to FD's 5 & 6, to avoid mixing up the > trace output with our own output under "-x". We do not need to pipe to file descriptors 5 and 6. Other file descriptors would do, either. We need to redirect away from 1 and 2, is what this sentence should say. And the hint about `-x` suggests that even that is not true that we need to redirect 1, either, just 2. And we would only need to redirect 2 with shells that do not understand `BASH_XTRACEFD`. It would be best not to mention `-x` at all. > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 74aa6384755..5fe2ea03c1d 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -20,15 +20,16 @@ test_xmllint () { > fi > } > > -test_lazy_prereq SYSTEMD_ANALYZE ' > - systemd-analyze verify /lib/systemd/system/basic.target > -' > - > 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. It is a bit sad that we're now doing unnecessary work when `systemd-analyze` does not even exist. > + > + cat systemd.out >&5 && > + sed -n \ > + -e '/^Failed to load/d' \ Lines starting with the prefix `Failed to load` are now deleted. Okay. Nothing in the commit explains why we can safely ignore those messages, though. > + -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. 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. > + <systemd.err >&6 && > + rm systemd.out systemd.err > } > > test_expect_success 'help text' ' > @@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' ' > # start registers the repo > git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > - test_systemd_analyze_verify "systemd/user/git-maintenance@.service" && > + # If we have a systemd-analyze on the system we can verify the > + # generated file. > + test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err && > + test_must_be_empty out && > + test_must_be_empty err && Since the function name has the suffix `_verify`, the verification part should be _inside_ that function, not outside. This patch is not clear enough to tell whether it actually fixes a regression worth fast-tracking into v2.34.0 or not. Ciao, Johannes > > printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > test_cmp expect args && > -- > 2.34.0.rc2.791.gdbfcf909579 > >
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. > Also, commit messages in git.git should not assume that every reader has a > profound knowledge of `systemd`. The commit message needs to do a better > job at explaining what is broken in the first place. If I need to explain that in any detail I don't think this can go forward. I don't really understand systemd well enough and don't have time to do so before the release. This change seems to work well enough, and fixes the issue. What's *really* going on with systemd here, is there a better way to do this? I have no idea. > The CI runs pass, after all, so it is unclear that there is anything > broken that would need fixing to begin with. Huh? It's unclear that we've got a portability-related breakage in git.git because CI passes? The CI environment is a fairly monolithic environment that tests some very common setups. It's useful as a basic testing canary, but it's pretty much useless for finding out if we've got any sort of portability issues beyond a basic test/compile on linux/OSXWindows. E.g. we routinely break builds and/or tests on the BSDs because of some Linux/Windows/OSX-specific assumptions in our code. CI will catch some of those, but not a large category of other issues. So aside from this fix I don't think the GitHub CI can tell us much if anything in this regard. > [...] >> We also need to pipe this output to FD's 5 & 6, to avoid mixing up the >> trace output with our own output under "-x". > > We do not need to pipe to file descriptors 5 and 6. Other file descriptors > would do, either. We need to redirect away from 1 and 2, is what this > sentence should say. Any other wouldn't do, because this is running in the context of test-lib.sh, which squat on some of the others from 3..9. Yeah, I should mention that, but you can't just pick any of 3..9 here. > And the hint about `-x` suggests that even that is not true that we need > to redirect 1, either, just 2. And we would only need to redirect 2 with > shells that do not understand `BASH_XTRACEFD`. It would be best not to > mention `-x` at all. I'd like both stdout & stderr here, and $ sh -c 'set -x; echo hi' >/dev/null + echo hi $ sh -c 'set -x; echo hi' 2>/dev/null hi I don't think we have a way in the tests to easily hoist the "turn off tracing" bit that test-lib.sh itself uses in similar cases, maybe I'm wrong and there's some easier way to do this. >> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh >> index 74aa6384755..5fe2ea03c1d 100755 >> --- a/t/t7900-maintenance.sh >> +++ b/t/t7900-maintenance.sh >> @@ -20,15 +20,16 @@ test_xmllint () { >> fi >> } >> >> -test_lazy_prereq SYSTEMD_ANALYZE ' >> - systemd-analyze verify /lib/systemd/system/basic.target >> -' >> - >> 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". > 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? Moving that to a test_have_prereq would just mean running it twice. Unless there's some better approach here I've missed. >> + >> + cat systemd.out >&5 && >> + sed -n \ >> + -e '/^Failed to load/d' \ > > Lines starting with the prefix `Failed to load` are now deleted. Okay. > Nothing in the commit explains why we can safely ignore those messages, > though. Quoting from it: To do that ignore whatever exit code we get from "systemd-analyze verify", and filter its stderr output to extract the sorts of lines it emits on note syntax warnings and errors. We need to filter out "Failed to load", which would be emitted e.g. on the gcc135.fsffrance.org test box[1]. Isn't that covered by that mention of "Failed to load"? Can you suggest a rewording you'd be happy with? >> + -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. >> + <systemd.err >&6 && >> + rm systemd.out systemd.err >> } >> >> test_expect_success 'help text' ' >> @@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' ' >> # start registers the repo >> git config --get --global --fixed-value maintenance.repo "$(pwd)" && >> >> - test_systemd_analyze_verify "systemd/user/git-maintenance@.service" && >> + # If we have a systemd-analyze on the system we can verify the >> + # generated file. >> + test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err && >> + test_must_be_empty out && >> + test_must_be_empty err && > > Since the function name has the suffix `_verify`, the verification part > should be _inside_ that function, not outside. > > This patch is not clear enough to tell whether it actually fixes a > regression worth fast-tracking into v2.34.0 or not. 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. I.e. as the commit message and the commits it links to explain we're really just asserting that when we change the systemd-specific code that we're not introducing syntax errors or whatever. So as long as this test runs for whoever is changing that (and they'd notice if it didn't) we're OK.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. >>> 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). >> 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". > + -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 ;-) > 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.
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 74aa6384755..5fe2ea03c1d 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -20,15 +20,16 @@ test_xmllint () { fi } -test_lazy_prereq SYSTEMD_ANALYZE ' - systemd-analyze verify /lib/systemd/system/basic.target -' - 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; + + cat systemd.out >&5 && + sed -n \ + -e '/^Failed to load/d' \ + -e '/git-maintenance@i*\.service:/x' \ + <systemd.err >&6 && + rm systemd.out systemd.err } test_expect_success 'help text' ' @@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' ' # start registers the repo git config --get --global --fixed-value maintenance.repo "$(pwd)" && - test_systemd_analyze_verify "systemd/user/git-maintenance@.service" && + # If we have a systemd-analyze on the system we can verify the + # generated file. + test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err && + test_must_be_empty out && + test_must_be_empty err && printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && test_cmp expect args &&
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. There was an attempt to work around previous breakage in these tests in 670e5973992 (maintenance: fix test t7900-maintenance.sh, 2021-09-27), as noted in my [1] that commit is wrong about its assumption that we can use "/lib/systemd/system/basic.target" as a canary.argument. To fix this let's adjust this test to test what it really should be testing: If we've got systemd-analyze reporting anything useful, we should use it to check the syntax of our just-generated "systemd/user/git-maintenance@.service" file. Even on systems where this previously succeeded we weren't effectively doing that, because "systemd-analyze" will pass various syntax errors by and exit with a status code of 0, e.g. if the "[Unit]" section is replaced with a nonsensical "[HlaghUnfUnf]" section. To do that ignore whatever exit code we get from "systemd-analyze verify", and filter its stderr output to extract the sorts of lines it emits on note syntax warnings and errors. We need to filter out "Failed to load", which would be emitted e.g. on the gcc135.fsffrance.org test box[1]. We also need to pipe this output to FD's 5 & 6, to avoid mixing up the trace output with our own output under "-x". 1. https://lore.kernel.org/git/211026.8635oo11jk.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- This test is still broken in rc2, this patch fixes it. The only update is a commit message grammar fix pointed out by Eric Sunshine, thanks! Range-diff against v1: 1: 90172a8ddcc ! 1: 44f0cafa16e maintenance tests: fix systemd v2.34.0-rc* test regression @@ Commit message maintenance tests: fix systemd v2.34.0-rc* test regression Fix tests added in b681b191f92 (maintenance: add support for systemd - timers on Linux, 2021-09-04) to run successfully no systems where + 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. @@ Commit message To do that ignore whatever exit code we get from "systemd-analyze verify", and filter its stderr output to extract the sorts of lines it - emits no note syntax warnings and errors. We need to filter out + emits on note syntax warnings and errors. We need to filter out "Failed to load", which would be emitted e.g. on the gcc135.fsffrance.org test box[1]. t/t7900-maintenance.sh | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)