[PATCH 0/4] test: T380 rework

[PATCH 0/4] test: T380 rework

[Michael J Gruber]

I've been testing with tests now (duh), i.e. enabling the tests for
regular Fedora distribution builds. This seems to be mostly OK, no flaky
fails so far. asan works locally (on x86_64) but we don't release build
with asan; and we don't have sfsexp in Fedora yet (working on it). All
other tests run.

In this context I noticed a few pecularities with T380. They all have to
do with the cases when either gdb is not available or fails to set
breakpoints, and this series tries to improve test output (and make
tests fail for the right reason).

The series is kind of micro-granular with commit messages longer than
the patch diff (in good old git.git fashion), to make it readable and
also partially-pickable if you prefer.

Michael J Gruber (4):
  test: correct comparison order in T380
  test: due not pass T380.1 for the wrong reasons
  test: reword T380.2 to be clearer
  test: set up the outcount file for T380.1

 test/T380-atomicity.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.35.1.306.ga00bde9711

[PATCH 1/4] test: correct comparison order in T380

[Michael J Gruber]

Specifying test comparisons as "expected actual" gives a better readable
diff since the "-" indicates missing, "+" additional items compared to
the expectations.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 test/T380-atomicity.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
index afe49d93..a6f1e037 100755
--- a/test/T380-atomicity.sh
+++ b/test/T380-atomicity.sh
@@ -93,7 +93,7 @@ if test_require_external_prereq gdb; then
 fi
 
 test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
-test_expect_equal_file searchall expectall
+test_expect_equal_file expectall searchall
 
 test_begin_subtest "detected $outcount>10 abort points"
 test_expect_success "test $outcount -gt 10"
-- 
2.35.1.306.ga00bde9711

[PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[Michael J Gruber]

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 <git@grubix.eu>
---
 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
+    outcount=0
 fi
 
 test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
-- 
2.35.1.306.ga00bde9711

[PATCH 3/4] test: reword T380.2 to be clearer

[Michael J Gruber]

T380.2 gives a test description which depends on the actual test output,
rather than the expected outcome or actual test which is performed.

So, when the test fails due missing abort points, the test describes
itself as `detected 0>10 abort points` so that it's not clear which part
or which number is the expectation. (Also, `0>10` is no number ...)

When the test is not run for some reason and fails because of that, the
test describes itself as `detected >10 abort points`, which arguably is
better or worse.

Reword it to say `detected more than 10 abort points`, which is the
actual expectation and what we test for. The failing test line still
gives the actual number.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 test/T380-atomicity.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
index 7f618062..49df5c38 100755
--- a/test/T380-atomicity.sh
+++ b/test/T380-atomicity.sh
@@ -99,7 +99,7 @@ fi
 test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
 test_expect_equal_file expectall searchall
 
-test_begin_subtest "detected $outcount>10 abort points"
+test_begin_subtest "detected more than 10 abort points"
 test_expect_success "test $outcount -gt 10"
 
 test_done
-- 
2.35.1.306.ga00bde9711

[PATCH 4/4] test: set up the outcount file for T380.1

[Michael J Gruber]

If gdb is present but for some reason `atomicity.py` fails to write to
the output file then the test fails with some ugly bash errors in the
wrong places (because the outcount variable is empty).

Therefore, set up the outcount file with `0` to get the test script to
rund and the test to fail fpr a clearer reason.

Background: We noticed this with arch armhfp emulated on x86_64 in
Fedora's COPR test build environment.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 test/T380-atomicity.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
index 49df5c38..caac28a3 100755
--- a/test/T380-atomicity.sh
+++ b/test/T380-atomicity.sh
@@ -64,6 +64,7 @@ if test_require_external_prereq gdb; then
     # -tty /dev/null works around a conflict between the 'timeout' wrapper
     # and gdb's attempt to control the TTY.
     export MAIL_DIR
+    echo -n 0 > outcount
     ${TEST_GDB} -tty /dev/null -batch -x $NOTMUCH_SRCDIR/test/atomicity.py notmuch 1>gdb.out 2>&1
 
     # Get the final, golden output
-- 
2.35.1.306.ga00bde9711

Re: [PATCH 1/4] test: correct comparison order in T380

[Tomi Ollila]

On Wed, Feb 09 2022, Michael J. Gruber wrote:

> Specifying test comparisons as "expected actual" gives a better readable
> diff since the "-" indicates missing, "+" additional items compared to
> the expectations.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  test/T380-atomicity.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
> index afe49d93..a6f1e037 100755
> --- a/test/T380-atomicity.sh
> +++ b/test/T380-atomicity.sh
> @@ -93,7 +93,7 @@ if test_require_external_prereq gdb; then
>  fi
>  
>  test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
> -test_expect_equal_file searchall expectall
> +test_expect_equal_file expectall searchall

Good in princible, but if things are like decade ago, we still have
hundreds of these in wrong order -- if so, all should be changed for
consistency. If I am wrong and all / many of those are already changed,
then this is OK.

>  
>  test_begin_subtest "detected $outcount>10 abort points"
>  test_expect_success "test $outcount -gt 10"
> -- 
> 2.35.1.306.ga00bde9711

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[Tomi Ollila]

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 <git@grubix.eu>
> ---
>  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

Re: [PATCH 3/4] test: reword T380.2 to be clearer

[Tomi Ollila]

On Wed, Feb 09 2022, Michael J. Gruber wrote:

> T380.2 gives a test description which depends on the actual test output,
> rather than the expected outcome or actual test which is performed.
>
> So, when the test fails due missing abort points, the test describes
> itself as `detected 0>10 abort points` so that it's not clear which part
> or which number is the expectation. (Also, `0>10` is no number ...)
>
> When the test is not run for some reason and fails because of that, the
> test describes itself as `detected >10 abort points`, which arguably is
> better or worse.
>
> Reword it to say `detected more than 10 abort points`, which is the
> actual expectation and what we test for. The failing test line still
> gives the actual number.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  test/T380-atomicity.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
> index 7f618062..49df5c38 100755
> --- a/test/T380-atomicity.sh
> +++ b/test/T380-atomicity.sh
> @@ -99,7 +99,7 @@ fi
>  test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
>  test_expect_equal_file expectall searchall
>  
> -test_begin_subtest "detected $outcount>10 abort points"
> +test_begin_subtest "detected more than 10 abort points"

probably ok (I trust the commit msg is right :D)

>  test_expect_success "test $outcount -gt 10"
>  
>  test_done
> -- 
> 2.35.1.306.ga00bde9711
>
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

Re: [PATCH 4/4] test: set up the outcount file for T380.1

[Tomi Ollila]

On Wed, Feb 09 2022, Michael J. Gruber wrote:

> If gdb is present but for some reason `atomicity.py` fails to write to
> the output file then the test fails with some ugly bash errors in the
> wrong places (because the outcount variable is empty).
>
> Therefore, set up the outcount file with `0` to get the test script to
> rund and the test to fail fpr a clearer reason.
>
> Background: We noticed this with arch armhfp emulated on x86_64 in
> Fedora's COPR test build environment.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  test/T380-atomicity.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
> index 49df5c38..caac28a3 100755
> --- a/test/T380-atomicity.sh
> +++ b/test/T380-atomicity.sh
> @@ -64,6 +64,7 @@ if test_require_external_prereq gdb; then
>      # -tty /dev/null works around a conflict between the 'timeout' wrapper
>      # and gdb's attempt to control the TTY.
>      export MAIL_DIR
> +    echo -n 0 > outcount

printf 0 > outcount (is my suggestion)

>      ${TEST_GDB} -tty /dev/null -batch -x $NOTMUCH_SRCDIR/test/atomicity.py notmuch 1>gdb.out 2>&1
>  
>      # Get the final, golden output
> -- 
> 2.35.1.306.ga00bde9711

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[David Bremner]

Michael J Gruber <git@grubix.eu> 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

Re: [PATCH 1/4] test: correct comparison order in T380

[Michael J Gruber]

Tomi Ollila venit, vidit, dixit 2022-02-09 21:47:46:
> On Wed, Feb 09 2022, Michael J. Gruber wrote:
> 
> > Specifying test comparisons as "expected actual" gives a better readable
> > diff since the "-" indicates missing, "+" additional items compared to
> > the expectations.
> >
> > Signed-off-by: Michael J Gruber <git@grubix.eu>
> > ---
> >  test/T380-atomicity.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
> > index afe49d93..a6f1e037 100755
> > --- a/test/T380-atomicity.sh
> > +++ b/test/T380-atomicity.sh
> > @@ -93,7 +93,7 @@ if test_require_external_prereq gdb; then
> >  fi
> >  
> >  test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
> > -test_expect_equal_file searchall expectall
> > +test_expect_equal_file expectall searchall
> 
> Good in princible, but if things are like decade ago, we still have
> hundreds of these in wrong order -- if so, all should be changed for
> consistency. If I am wrong and all / many of those are already changed,
> then this is OK.

I haven't checked, but the order add to the confusion created by
"detected >10 abort points" which is why I changed it here, and in order
to (mis-)use these files in the following commits.

> >  
> >  test_begin_subtest "detected $outcount>10 abort points"
> >  test_expect_success "test $outcount -gt 10"
> > -- 
> > 2.35.1.306.ga00bde9711

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[Michael J Gruber]

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 <git@grubix.eu>
> > ---
> >  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

Re: [PATCH 4/4] test: set up the outcount file for T380.1

[Michael J Gruber]

Tomi Ollila venit, vidit, dixit 2022-02-09 21:50:35:
> On Wed, Feb 09 2022, Michael J. Gruber wrote:
> 
> > If gdb is present but for some reason `atomicity.py` fails to write to
> > the output file then the test fails with some ugly bash errors in the
> > wrong places (because the outcount variable is empty).
> >
> > Therefore, set up the outcount file with `0` to get the test script to
> > rund and the test to fail fpr a clearer reason.
> >
> > Background: We noticed this with arch armhfp emulated on x86_64 in
> > Fedora's COPR test build environment.
> >
> > Signed-off-by: Michael J Gruber <git@grubix.eu>
> > ---
> >  test/T380-atomicity.sh | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
> > index 49df5c38..caac28a3 100755
> > --- a/test/T380-atomicity.sh
> > +++ b/test/T380-atomicity.sh
> > @@ -64,6 +64,7 @@ if test_require_external_prereq gdb; then
> >      # -tty /dev/null works around a conflict between the 'timeout' wrapper
> >      # and gdb's attempt to control the TTY.
> >      export MAIL_DIR
> > +    echo -n 0 > outcount
> 
> printf 0 > outcount (is my suggestion)

Would by mine, too :)
Again, this is not the style of T380 which uses echo exclusively so far.

I do think that printf is more stable across different systems and
shells, so a mass replace would be in order (but requires careful nl
checking).

Overally in the test suite, [f]printf wins by 8:3 roughly.

> 
> >      ${TEST_GDB} -tty /dev/null -batch -x $NOTMUCH_SRCDIR/test/atomicity.py notmuch 1>gdb.out 2>&1
> >  
> >      # Get the final, golden output
> > -- 
> > 2.35.1.306.ga00bde9711

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[Michael J Gruber]

David Bremner venit, vidit, dixit 2022-02-09 23:12:24:
> Michael J Gruber <git@grubix.eu> 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

Re: [PATCH 4/4] test: set up the outcount file for T380.1

[Tomi Ollila]

On Thu, Feb 10 2022, Michael J. Gruber wrote:

> Tomi Ollila venit, vidit, dixit 2022-02-09 21:50:35:
>> On Wed, Feb 09 2022, Michael J. Gruber wrote:
>> 
>> > If gdb is present but for some reason `atomicity.py` fails to write to
>> > the output file then the test fails with some ugly bash errors in the
>> > wrong places (because the outcount variable is empty).
>> >
>> > Therefore, set up the outcount file with `0` to get the test script to
>> > rund and the test to fail fpr a clearer reason.
>> >
>> > Background: We noticed this with arch armhfp emulated on x86_64 in
>> > Fedora's COPR test build environment.
>> >
>> > Signed-off-by: Michael J Gruber <git@grubix.eu>
>> > ---
>> >  test/T380-atomicity.sh | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
>> > index 49df5c38..caac28a3 100755
>> > --- a/test/T380-atomicity.sh
>> > +++ b/test/T380-atomicity.sh
>> > @@ -64,6 +64,7 @@ if test_require_external_prereq gdb; then
>> >      # -tty /dev/null works around a conflict between the 'timeout' wrapper
>> >      # and gdb's attempt to control the TTY.
>> >      export MAIL_DIR
>> > +    echo -n 0 > outcount
>> 
>> printf 0 > outcount (is my suggestion)
>
> Would by mine, too :)
> Again, this is not the style of T380 which uses echo exclusively so far.

Right, I did not see the surrouding code in this series :D

(the only thing I saw use of $(expr ...) -- which I put to my notes of
trivial fixes one could do in (distant?) future)

Perhaps the series is good then (and perhaps incremental changing of
expected - actual order is better than newer changing any of those)

Tomi

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[David Bremner]

Michael J Gruber <git@grubix.eu> 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

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[Tomi Ollila]

On Sat, Feb 12 2022, David Bremner wrote:

> Michael J Gruber <git@grubix.eu> 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

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> 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

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[Michael J Gruber]

David Bremner venit, vidit, dixit 2022-02-20 00:02:40:
> Tomi Ollila <tomi.ollila@iki.fi> 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.

Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons

[David Bremner]

Michael J Gruber <git@grubix.eu> 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

Re: [PATCH 0/4] test: T380 rework

[Michael J Gruber]

[-- Attachment #1.1: Type: text/plain, Size: 1457 bytes --]

So, with the key-value pairs sorted by both, I resumed testing for Python
3.1.13 and encountered failing T380 which gave me some a deja-vue due to
its confusing messages:

```
T380-atomicity: Testing atomicity
cat: outcount: No such file or directory
/builddir/build/BUILD/notmuch-0.38.1/test/T380-atomicity.sh: line 79: ((: i
< : syntax error: operand expected (error token is "< ")
 PASS   "notmuch new" is idempotent under arbitrary aborts
 FAIL   detected >10 abort points
        test  -gt 10
/builddir/build/BUILD/notmuch-0.38.1/test/test-lib.sh: line 701: test: -gt:
unary operator expected
```

And that is why this is a reply to the old thread where I suggested making
this less confusing, because everything reported is not the actual failure,
but the consequence of not safe-guarding against a failed test setup.

The *real cause* is most likely that `import gdb` fails in `atomicity.py`
because it's not ready for py 3.13 yet.

But maybe it's time to reconsider some bits of the old series? We ended up
discussing "echo vs printf" and never addressed the real issues here.

Cheers,
Michael

P.S.: There are also a few lines like
```
Error: database path
'/builddir/build/BUILD/notmuch-0.38.1/test/tmp.T400-hooks/database.85' does
not exist or is not a directory.
```
sprinkled in the test output between PASS tests, apparently without making
any test fail. I don't know whether I never noticed for a apassing test
suite, or whether this is new.

[-- Attachment #1.2: Type: text/html, Size: 1795 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/4] test: T380 rework

[Tomi Ollila]

On Fri, Nov 24 2023, Michael J. Gruber wrote:

> So, with the key-value pairs sorted by both, I resumed testing for Python
> 3.1.13 and encountered failing T380 which gave me some a deja-vue due to
> its confusing messages:
>
> ```
> T380-atomicity: Testing atomicity
> cat: outcount: No such file or directory
> /builddir/build/BUILD/notmuch-0.38.1/test/T380-atomicity.sh: line 79: ((: i
> < : syntax error: operand expected (error token is "< ")

Minimal change would be to change line:

    for ((i = 0; i < $outcount; i++)); do

as

    for ((i = 0; i < ${outcount:=0}; i++)); do

that would change the empty ('') $outcount to be as '0', and also
return the same value (0) there in expression.

Then, at least the message is less confusing...

Tomi

>  PASS   "notmuch new" is idempotent under arbitrary aborts
>  FAIL   detected >10 abort points
>         test  -gt 10
> /builddir/build/BUILD/notmuch-0.38.1/test/test-lib.sh: line 701: test: -gt:
> unary operator expected
> ```
>
> And that is why this is a reply to the old thread where I suggested making
> this less confusing, because everything reported is not the actual failure,
> but the consequence of not safe-guarding against a failed test setup.
>
> The *real cause* is most likely that `import gdb` fails in `atomicity.py`
> because it's not ready for py 3.13 yet.
>
> But maybe it's time to reconsider some bits of the old series? We ended up
> discussing "echo vs printf" and never addressed the real issues here.
>
> Cheers,
> Michael
>
> P.S.: There are also a few lines like
> ```
> Error: database path
> '/builddir/build/BUILD/notmuch-0.38.1/test/tmp.T400-hooks/database.85' does
> not exist or is not a directory.
> ```
> sprinkled in the test output between PASS tests, apparently without making
> any test fail. I don't know whether I never noticed for a apassing test
> suite, or whether this is new.
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org