[PATCH] test: Ignore subtly different behaviour of the musl libc

[PATCH] test: Ignore subtly different behaviour of the musl libc

[Luis Ressel]

---
 test/T030-config.sh       | 6 ++++--
 test/T650-regexp-query.sh | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/test/T030-config.sh b/test/T030-config.sh
index f36695c6..9a384f1e 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -43,7 +43,7 @@ notmuch config set foo.nonexistent
 test_expect_equal "$(notmuch config get foo.nonexistent)" ""
 
 test_begin_subtest "List all items"
-notmuch config list 2>&1 | notmuch_config_sanitize > OUTPUT
+notmuch config list 2>OUTPUT-ERR | notmuch_config_sanitize > OUTPUT
 
 if [ "${NOTMUCH_GMIME_MAJOR}" -lt 3 ]; then
     config_gpg_path="crypto.gpg_path=gpg
@@ -51,7 +51,6 @@ if [ "${NOTMUCH_GMIME_MAJOR}" -lt 3 ]; then
 fi
 
 cat <<EOF > EXPECTED
-Error opening database at MAIL_DIR/.notmuch: No such file or directory
 database.path=MAIL_DIR
 user.name=Notmuch Test Suite
 user.primary_email=test_suite@notmuchmail.org
@@ -68,6 +67,9 @@ built_with.retry_lock=something
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "List all items (stderr output)"
+test_expect_equal "$(notmuch_config_sanitize <OUTPUT-ERR)" "Error opening database at MAIL_DIR/.notmuch: No such file or directory"
+
 test_begin_subtest "Top level --config=FILE option"
 cp "${NOTMUCH_CONFIG}" alt-config
 notmuch --config=alt-config config set user.name "Another Name"
diff --git a/test/T650-regexp-query.sh b/test/T650-regexp-query.sh
index 4085340f..9ba3cd64 100755
--- a/test/T650-regexp-query.sh
+++ b/test/T650-regexp-query.sh
@@ -137,10 +137,10 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "regexp error reporting"
-notmuch search 'from:/unbalanced[/' 1>OUTPUT 2>&1
+notmuch search 'from:/unbalanced[/' 2>&1 | sed -e 's/^\(A Xapian[^:]*:\).*/\1/' > OUTPUT
 cat <<EOF > EXPECTED
 notmuch search: A Xapian exception occurred
-A Xapian exception occurred parsing query: Invalid regular expression
+A Xapian exception occurred parsing query:
 Query string was: from:/unbalanced[/
 EOF
 test_expect_equal_file EXPECTED OUTPUT
-- 
2.19.2

Re: [PATCH] test: Ignore subtly different behaviour of the musl libc

[David Bremner]

Luis Ressel <aranea@aixah.de> writes:

> ---
>  test/T030-config.sh       | 6 ++++--
>  test/T650-regexp-query.sh | 4 ++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>

In general we want more verbose commit messages for notmuch
(https://notmuchmail.org/contributing/#index5h2)

> +test_begin_subtest "List all items (stderr output)"
> +test_expect_equal "$(notmuch_config_sanitize <OUTPUT-ERR)" "Error opening database at MAIL_DIR/.notmuch: No such file or directory"
> +

The first change looks OK. Another option would be to cat the two files
into one in the test with a seperator. That's how the test_C based tests work.

>  test_begin_subtest "Top level --config=FILE option"
>  cp "${NOTMUCH_CONFIG}" alt-config
>  notmuch --config=alt-config config set user.name "Another Name"
> diff --git a/test/T650-regexp-query.sh b/test/T650-regexp-query.sh
> index 4085340f..9ba3cd64 100755
> --- a/test/T650-regexp-query.sh
> +++ b/test/T650-regexp-query.sh
> @@ -137,10 +137,10 @@ EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
>  test_begin_subtest "regexp error reporting"
> -notmuch search 'from:/unbalanced[/' 1>OUTPUT 2>&1
> +notmuch search 'from:/unbalanced[/' 2>&1 | sed -e 's/^\(A Xapian[^:]*:\).*/\1/' > OUTPUT
>  cat <<EOF > EXPECTED
>  notmuch search: A Xapian exception occurred
> -A Xapian exception occurred parsing query: Invalid regular expression
> +A Xapian exception occurred parsing query:
>  Query string was: from:/unbalanced[/
>  EOF

This seems to lose the fact that a regexp parsing error occured.  One
option would be to change the actual error message so the initial
predictable part of the error message contained some string like
"regexp".

d

Re: [PATCH] test: Ignore subtly different behaviour of the musl libc

[Luis Ressel]

On Mon, Mar 04, 2019 at 08:04:21AM -0400, David Bremner wrote:
> Luis Ressel <aranea@aixah.de> writes:
> 
> > ---
> >  test/T030-config.sh       | 6 ++++--
> >  test/T650-regexp-query.sh | 4 ++--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> >
> 
> In general we want more verbose commit messages for notmuch
> (https://notmuchmail.org/contributing/#index5h2)
> 
> > +test_begin_subtest "List all items (stderr output)"
> > +test_expect_equal "$(notmuch_config_sanitize <OUTPUT-ERR)" "Error opening database at MAIL_DIR/.notmuch: No such file or directory"
> > +
> 
> The first change looks OK. Another option would be to cat the two files
> into one in the test with a seperator. That's how the test_C based tests work.
> 

I agree that's nicer, and just submitted an updated patch.

> >  test_begin_subtest "Top level --config=FILE option"
> >  cp "${NOTMUCH_CONFIG}" alt-config
> >  notmuch --config=alt-config config set user.name "Another Name"
> > diff --git a/test/T650-regexp-query.sh b/test/T650-regexp-query.sh
> > index 4085340f..9ba3cd64 100755
> > --- a/test/T650-regexp-query.sh
> > +++ b/test/T650-regexp-query.sh
> > @@ -137,10 +137,10 @@ EOF
> >  test_expect_equal_file EXPECTED OUTPUT
> >  
> >  test_begin_subtest "regexp error reporting"
> > -notmuch search 'from:/unbalanced[/' 1>OUTPUT 2>&1
> > +notmuch search 'from:/unbalanced[/' 2>&1 | sed -e 's/^\(A Xapian[^:]*:\).*/\1/' > OUTPUT
> >  cat <<EOF > EXPECTED
> >  notmuch search: A Xapian exception occurred
> > -A Xapian exception occurred parsing query: Invalid regular expression
> > +A Xapian exception occurred parsing query:
> >  Query string was: from:/unbalanced[/
> >  EOF
> 
> This seems to lose the fact that a regexp parsing error occured.  One
> option would be to change the actual error message so the initial
> predictable part of the error message contained some string like
> "regexp".

I don't really think this is a problem. notmuch's test suite is quite
comprehensive, so I don't consider it essential to verify that this
particular Xapian failure is caused by a regex issue as opposed to
another problem.

I could change the test so it accepts both the glibc and musl errors
("Invalid regular expression" and "Missing ']'", respectively), but I
can't say I'm enthusiastic about that. POSIX does not specify the output
of regerror(), and in fact not even which error code should be used here
(musl's regcomp() currently returns REG_EBRACK, while glibc probably
uses the generic REG_BADPAT); thus, the error message may change
arbitrarily during a libc update, or be something else entirely for
another libc implementation.

If you really consider it important to convey the info that there was a
regex parsing error, I can submit a patch to make compile_regex() prefix
the regerror output with "regex error: " or sth like that.

Regards,
Luis Ressel

(Resending due to trouble with my new MUA; apologies if you receive this
mail twice.)

Re: [PATCH] test: Ignore subtly different behaviour of the musl libc

[Tomi Ollila]

On Tue, Feb 26 2019, Luis Ressel wrote:
...
> --- a/test/T650-regexp-query.sh
> +++ b/test/T650-regexp-query.sh
> @@ -137,10 +137,10 @@ EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
>  test_begin_subtest "regexp error reporting"
> -notmuch search 'from:/unbalanced[/' 1>OUTPUT 2>&1
> +notmuch search 'from:/unbalanced[/' 2>&1 | sed -e 's/^\(A Xapian[^:]*:\).*/\1/' > OUTPUT

Simpler sed:  sed '/A Xapian/ s/:.*/:/'

>  cat <<EOF > EXPECTED
>  notmuch search: A Xapian exception occurred
> -A Xapian exception occurred parsing query: Invalid regular expression
> +A Xapian exception occurred parsing query:
>  Query string was: from:/unbalanced[/
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT

Re: [PATCH] test: Ignore subtly different behaviour of the musl libc

[David Bremner]

Luis Ressel <aranea@aixah.de> writes:

> If you really consider it important to convey the info that there was a
> regex parsing error, I can submit a patch to make compile_regex() prefix
> the regerror output with "regex error: " or sth like that.

That sounds good. The exception loses all location information at the C
boundary, so the message is all we have. The test was originally added
to make sure the regexp error checking (such as it is) was working, and
I'd like to preserve that if possible.

d