Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
Michael J Gruber writes: > Alternatively, require GNU coreutils and use /usr/bin/echo. > Right, we're try to avoid a hard dependency on coreutils in the test suite. Note that the test suite requires bash >= 4, so portability of the builtins is less of a practical issue. d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
David Bremner venit, vidit, dixit 2022-02-20 00:02:40: > Tomi Ollila writes: > > > > > Wat? afaik echo is builtin in every modern bourne shell derivative... > > > > (I tested: > > $ bash -c 'builtin echo foo' > > foo > > $ bash -c 'export PATH=/tmp; echo foo; ls' > > foo > > bash: ls: command not found > > ) > > Oops. That's what I get for believing "which". Which is another tale of > woe, of course. Builtin in zsh, and not in bash. Not that it matters > here, but probably why it doesn't know about bash builtins. echo is both a builtin (for bash etc) and a command: $ type echo echo is a shell builtin $ which echo /usr/bin/echo The latter is part of GNU coreutils (sh-utils) and often used when one wants to avoid shell pecularities. printf is a builtin Due to the way the builtins were specified (or underspecified in the case of echo) in POSIX, the printf builtin is more portable than the echo builtin. As a rule of thumb, the echo builtin is OK if you don't need options (nor special control sequences) but want a newline at the end of the output anyways; use printf if you need options or format strings. Alternatively, require GNU coreutils and use /usr/bin/echo. Cheers Michael P.S.: I haven't forgotten about the series but it needs quite a bit of rework as we found out. ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
Tomi Ollila writes: > > Wat? afaik echo is builtin in every modern bourne shell derivative... > > (I tested: > $ bash -c 'builtin echo foo' > foo > $ bash -c 'export PATH=/tmp; echo foo; ls' > foo > bash: ls: command not found > ) Oops. That's what I get for believing "which". Which is another tale of woe, of course. Builtin in zsh, and not in bash. Not that it matters here, but probably why it doesn't know about bash builtins. d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
On Sat, Feb 12 2022, David Bremner wrote: > Michael J Gruber writes: > > >> When analysing this, I was confused by the way >> test_require_external_prereq works and the "if" in T380 (as opposed to how >> test_require_external_prereq is used in other tests). Over at git.git, >> we have test setup code in functions which don't get executed if >> prerequisites fail. I guess the "if" emulates that, but then the actual >> tests in T380 are outside the if block and use files and variables which >> are created in the if block. So, this is something to fix anyways. > > agreed. > >> Add to this the fact that the tests needing sfsexp or asan (and probably >> others) do things yet differently and call "test_done" immediately, so >> that no SKIP appears. And those were the only ones skipped at all here ... >> > > I think that's probably my fault for also not really understanding the > prereq system. > > >> In the short run, initialising variables and files which are used is >> still a good thing, but I would have to rewrite some commit messages. > > sure. > >> I'll wait until it's clear how to handle style, though: switch to printf >> from echo whenever I touch those lines (leading to mixed use) or keeing >> style and leaving the style change for another series. > > I think I lean to fixing the usage of echo -n incrementally (i.e. don't > introduce more). It might be a bit uglier in the short term, but > eventually we'll get there. If there currently is zero printf's and only echo, I'd personally continue to use echos -- but either way is ok by me > It turns out that echo is _not_ builtin in bash, so this really is a > portability bug. Wat? afaik echo is builtin in every modern bourne shell derivative... (I tested: $ bash -c 'builtin echo foo' foo $ bash -c 'export PATH=/tmp; echo foo; ls' foo bash: ls: command not found ) Tomi > > d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
Michael J Gruber writes: > When analysing this, I was confused by the way > test_require_external_prereq works and the "if" in T380 (as opposed to how > test_require_external_prereq is used in other tests). Over at git.git, > we have test setup code in functions which don't get executed if > prerequisites fail. I guess the "if" emulates that, but then the actual > tests in T380 are outside the if block and use files and variables which > are created in the if block. So, this is something to fix anyways. agreed. > Add to this the fact that the tests needing sfsexp or asan (and probably > others) do things yet differently and call "test_done" immediately, so > that no SKIP appears. And those were the only ones skipped at all here ... > I think that's probably my fault for also not really understanding the prereq system. > In the short run, initialising variables and files which are used is > still a good thing, but I would have to rewrite some commit messages. sure. > I'll wait until it's clear how to handle style, though: switch to printf > from echo whenever I touch those lines (leading to mixed use) or keeing > style and leaving the style change for another series. I think I lean to fixing the usage of echo -n incrementally (i.e. don't introduce more). It might be a bit uglier in the short term, but eventually we'll get there. It turns out that echo is _not_ builtin in bash, so this really is a portability bug. d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
David Bremner venit, vidit, dixit 2022-02-09 23:12:24: > Michael J Gruber writes: > > > If gdb is missing then some files are never written to so that the > > comparisons of non-existing files succeeds for the wrong reason, > > claiming that `notmch new` is idempotent when it was in fact never run. > > > > Catch this and (for lack of a better spot) set up the files with a > > reason for the FAIL. > > > > I'm a bit confused by this. For me if gdb is missing I get > > missing prerequisites: gdb(1) > SKIP all tests in T380-atomicity > > ... > > All 1842 tests behaved as expected (8 expected failures). > All tests in 1 file skipped. > > Do I misunderstand something, or is prerequisite detection not working > for you? It does now, my understanding has been mislead by a few things. The issues were: On one of Fedora's arches which is emulated in the COPR test environment, gdb was present (just like in all of them) but apparantly failed to set any break points. T380 spew out something like cat: outcount: No such file or directory ./T380-atomicity.sh: line 79: ((: i < : syntax error: operand expected (error token is "< ") followed by wrong PASS/FAIL etc. When analysing this, I was confused by the way test_require_external_prereq works and the "if" in T380 (as opposed to how test_require_external_prereq is used in other tests). Over at git.git, we have test setup code in functions which don't get executed if prerequisites fail. I guess the "if" emulates that, but then the actual tests in T380 are outside the if block and use files and variables which are created in the if block. So, this is something to fix anyways. In order to try things out locally, I changed if test_require_external_prereq gdb; then into if test_require_external_prereq gggdb; then I know now that `test_require_external_prereq binary` does not test for the presence of binary, nor does it abort the test script. It merely checks among the set of "predefined external prerequisites", and this would be much clearer if "binary" wasn't an argument to that function but a suffix: `test_require_external_prereq_gdb` immediately says "I'm something predefined". Until now, I wondered how `test_require_external_prereq` could skip the PASS/FAIL outout if it could not skip the previous part of the test script. This is not clear from its implementation. I know now that indeed text_expect-success etc. call test_check_missing_external_prereqs_ for the current subtest and skip the output. Add to this the fact that the tests needing sfsexp or asan (and probably others) do things yet differently and call "test_done" immediately, so that no SKIP appears. And those were the only ones skipped at all here ... In effect, I knoew SKIP only from manually skipping tests (NOTMUCH_SKIP_TESTS). I'm not complaining, on the other hand I don't fail too bad about myself for being confused by this ;) So, in the long run I think test_setup() is worth thinking about. In the short run, initialising variables and files which are used is still a good thing, but I would have to rewrite some commit messages. I'll wait until it's clear how to handle style, though: switch to printf from echo whenever I touch those lines (leading to mixed use) or keeing style and leaving the style change for another series. Cheers, Michael ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
Tomi Ollila venit, vidit, dixit 2022-02-09 21:49:18: > On Wed, Feb 09 2022, Michael J. Gruber wrote: > > > If gdb is missing then some files are never written to so that the > > comparisons of non-existing files succeeds for the wrong reason, > > claiming that `notmch new` is idempotent when it was in fact never run. > > > > Catch this and (for lack of a better spot) set up the files with a > > reason for the FAIL. > > > > Signed-off-by: Michael J Gruber > > --- > > test/T380-atomicity.sh | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh > > index a6f1e037..7f618062 100755 > > --- a/test/T380-atomicity.sh > > +++ b/test/T380-atomicity.sh > > @@ -90,6 +90,10 @@ if test_require_external_prereq gdb; then > > i=$(expr $end - 1) > > fi > > done > > +else > > +echo -n "Test fails due to missing gdb." > searchall > > +echo -n > expectall > > I am not much of a fan of 'echo -n' (I remember seeing -n (and newline > echoed...), therefore first to use printf and second : > expectall > (unless printf '' > expectall) I'm in favour of using printf - usually I don't impose my favours but follow surrounding style, which is exactly what I did here. In the if-block before the else, the files are writen to using "echo -n". I would be weird to do it differently in both blocks. > > > +outcount=0 > > fi > > > > test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' > > -- > > 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
Michael J Gruber writes: > If gdb is missing then some files are never written to so that the > comparisons of non-existing files succeeds for the wrong reason, > claiming that `notmch new` is idempotent when it was in fact never run. > > Catch this and (for lack of a better spot) set up the files with a > reason for the FAIL. > I'm a bit confused by this. For me if gdb is missing I get missing prerequisites: gdb(1) SKIP all tests in T380-atomicity ... All 1842 tests behaved as expected (8 expected failures). All tests in 1 file skipped. Do I misunderstand something, or is prerequisite detection not working for you? d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
On Wed, Feb 09 2022, Michael J. Gruber wrote: > If gdb is missing then some files are never written to so that the > comparisons of non-existing files succeeds for the wrong reason, > claiming that `notmch new` is idempotent when it was in fact never run. > > Catch this and (for lack of a better spot) set up the files with a > reason for the FAIL. > > Signed-off-by: Michael J Gruber > --- > test/T380-atomicity.sh | 4 > 1 file changed, 4 insertions(+) > > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh > index a6f1e037..7f618062 100755 > --- a/test/T380-atomicity.sh > +++ b/test/T380-atomicity.sh > @@ -90,6 +90,10 @@ if test_require_external_prereq gdb; then > i=$(expr $end - 1) > fi > done > +else > +echo -n "Test fails due to missing gdb." > searchall > +echo -n > expectall I am not much of a fan of 'echo -n' (I remember seeing -n (and newline echoed...), therefore first to use printf and second : > expectall (unless printf '' > expectall) > +outcount=0 > fi > > test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' > -- > 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
