[PATCH 3/3] tests: run all tests in parallel, if available

[PATCH 3/3] tests: run all tests in parallel, if available

[Rollins, Jameson]

From: Jameson Graef Rollins <jrollins@finestructure.net>

If either the moreutils or GNU parallel utility are available, run all
tests in parallel.  On my eight core machine this makes for a ~x7
speed-up in the full test suite (1m24s -> 12s).

The design of the test suite makes this parallelization trivial.
---
 test/notmuch-test | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/test/notmuch-test b/test/notmuch-test
index 1a1ae811..a4b7a1eb 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -40,17 +40,27 @@ fi
 
 trap 'e=$?; kill $!; exit $e' HUP INT TERM
 # Run the tests
-for test in $TESTS; do
-    $TEST_TIMEOUT_CMD $test "$@" &
-    wait $!
-    # If the test failed without producing results, then it aborted,
-    # so we should abort, too.
-    RES=$?
-    testname=$(basename $test .sh)
-    if [[ $RES != 0 && ! -e "$NOTMUCH_BUILDDIR/test/test-results/$testname" ]]; then
-        exit $RES
+if command -v parallel >/dev/null ; then
+    if parallel -h | grep -q GNU ; then
+        echo "INFO: running tests with GNU parallel"
+        printf '%s\n' $TESTS | $TEST_TIMEOUT_CMD parallel
+    else
+        echo "INFO: running tests with moreutils parallel"
+        $TEST_TIMEOUT_CMD parallel -- $TESTS
     fi
-done
+else
+    for test in $TESTS; do
+        $TEST_TIMEOUT_CMD $test "$@" &
+        wait $!
+        # If the test failed without producing results, then it aborted,
+        # so we should abort, too.
+        RES=$?
+        testname=$(basename $test .sh)
+        if [[ $RES != 0 && ! -e "$NOTMUCH_BUILDDIR/test/test-results/$testname" ]]; then
+            exit $RES
+        fi
+    done
+fi
 trap - HUP INT TERM
 
 # Report results
-- 
2.20.1

parallelize test suite

[Rollins, Jameson]

This is a simple patch series that will run the entire test suite in
parallel if either the moreutils or GNU parallel utility is
available.  On my 8-core machine the full test suite will now run in
under 20 seconds, which is a pretty huge improvement.

jamie.

[PATCH 2/3] tests: remove entangling corpus index optimization

[Rollins, Jameson]

From: Jameson Graef Rollins <jrollins@finestructure.net>

The add_email_corpus test utility includes logic that tries to re-use
an index of the corpus if available.  This was seemingly done as an
optimization, so that every test that uses the corpus didn't have to
create it's own index of the corpus.  However, this has the perverse
side effect of entangling tests together, and breaks parallelization.

Forcing each test to do it's own index does increase the overall time
of the test slightly (~6%), but this will be more than made up for in
the next patch that introduces paraellization.
---
 test/.gitignore     |  1 -
 test/Makefile.local |  2 +-
 test/notmuch-test   |  2 +-
 test/test-lib.sh    | 10 ++--------
 4 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/test/.gitignore b/test/.gitignore
index 69080e5e..f5968404 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -1,5 +1,4 @@
 /arg-test
-/corpora.mail
 /hex-xcode
 /parse-time
 /random-corpus
diff --git a/test/Makefile.local b/test/Makefile.local
index 1cf09778..47244e8f 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -81,4 +81,4 @@ check: test
 SRCS := $(SRCS) $(test_srcs)
 CLEAN += $(TEST_BINARIES) $(addsuffix .o,$(TEST_BINARIES)) \
 	 $(dir)/database-test.o \
-	 $(dir)/corpora.mail $(dir)/test-results $(dir)/tmp.*
+	 $(dir)/test-results $(dir)/tmp.*
diff --git a/test/notmuch-test b/test/notmuch-test
index ca68dd41..1a1ae811 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -59,6 +59,6 @@ $NOTMUCH_SRCDIR/test/aggregate-results.sh $NOTMUCH_BUILDDIR/test/test-results/*
 ev=$?
 
 # Clean up
-rm -rf $NOTMUCH_BUILDDIR/test/test-results $NOTMUCH_BUILDDIR/test/corpora.mail
+rm -rf $NOTMUCH_BUILDDIR/test/test-results
 
 exit $ev
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 04d93f7d..43339902 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -394,14 +394,8 @@ add_email_corpus ()
     corpus=${1:-default}
 
     rm -rf ${MAIL_DIR}
-    if [ -d $TEST_DIRECTORY/corpora.mail/$corpus ]; then
-	cp -a $TEST_DIRECTORY/corpora.mail/$corpus ${MAIL_DIR}
-    else
-	cp -a $NOTMUCH_SRCDIR/test/corpora/$corpus ${MAIL_DIR}
-	notmuch new >/dev/null || die "'notmuch new' failed while adding email corpus"
-	mkdir -p $TEST_DIRECTORY/corpora.mail
-	cp -a ${MAIL_DIR} $TEST_DIRECTORY/corpora.mail/$corpus
-    fi
+    cp -a $NOTMUCH_SRCDIR/test/corpora/$corpus ${MAIL_DIR}
+    notmuch new >/dev/null || die "'notmuch new' failed while adding email corpus"
 }
 
 test_begin_subtest ()
-- 
2.20.1

[PATCH 1/3] tests: remove some redundant pre-cleanup of the corpus MAIL_DIR

[Rollins, Jameson]

From: Jameson Graef Rollins <jrollins@finestructure.net>

add_email_corpus itself does an rm -rf $MAIL_DIR, so these are not necessary.
---
 test/T100-search-by-folder.sh | 1 -
 test/T650-regexp-query.sh     | 1 -
 2 files changed, 2 deletions(-)

diff --git a/test/T100-search-by-folder.sh b/test/T100-search-by-folder.sh
index a090f3d2..409cfdcc 100755
--- a/test/T100-search-by-folder.sh
+++ b/test/T100-search-by-folder.sh
@@ -56,7 +56,6 @@ output=$(notmuch search folder:duplicate/bad/olds | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Bears (inbox unread)"
 
 # folder: and path: searches with full corpus
-rm -rf $MAIL_DIR
 add_email_corpus
 
 # add some more dupes
diff --git a/test/T650-regexp-query.sh b/test/T650-regexp-query.sh
index 92334ba0..43af3b47 100755
--- a/test/T650-regexp-query.sh
+++ b/test/T650-regexp-query.sh
@@ -47,7 +47,6 @@ output=$(notmuch search 'path:/^bad$/' | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; To the bone (inbox unread)"
 
 # Use "standard" corpus from here on.
-rm -rf $MAIL_DIR
 add_email_corpus
 
 notmuch search --output=messages from:cworth > cworth.msg-ids
-- 
2.20.1

Re: parallelize test suite

[Rollins, Jameson]

On Sat, May 04 2019, "Rollins, Jameson" <jrollins@caltech.edu> wrote:
> This is a simple patch series that will run the entire test suite in
> parallel if either the moreutils or GNU parallel utility is
> available.  On my 8-core machine the full test suite will now run in
> under 20 seconds, which is a pretty huge improvement.

Sorry I actually meant 12 seconds, not 20.

Re: parallelize test suite

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On Sat 2019-05-04 20:57:43 +0000, Rollins, Jameson wrote:
> This is a simple patch series that will run the entire test suite in
> parallel if either the moreutils or GNU parallel utility is
> available.  On my 8-core machine the full test suite will now run in
> under 20 seconds, which is a pretty huge improvement.

I've reviewed this series, and it is a *very* nice cleanup; the latency
reduction makes it much more pleasant to develop notmuch safely.

My computer is weaker than jamie's (i have an older 4-core i5
processor), but with this series applied, the test suite went from 1m48s
to 0m29s for me.

Please apply this series!

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: parallelize test suite

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> On Sat 2019-05-04 20:57:43 +0000, Rollins, Jameson wrote:
>> This is a simple patch series that will run the entire test suite in
>> parallel if either the moreutils or GNU parallel utility is
>> available.  On my 8-core machine the full test suite will now run in
>> under 20 seconds, which is a pretty huge improvement.
>
> I've reviewed this series, and it is a *very* nice cleanup; the latency
> reduction makes it much more pleasant to develop notmuch safely.
>
> My computer is weaker than jamie's (i have an older 4-core i5
> processor), but with this series applied, the test suite went from 1m48s
> to 0m29s for me.
>

Last time we discussed parallel test running, there we concerns about
multiple versions of certain servers colliding with each other. This
still seems to be at least a theoretical issue with smtp-dummy, although
a glance suggests that it might only currently be used in T310-emacs.sh.
I'm not sure what a robust solution is here. 

- gpg-agent
- emacs
- dtach

Did I miss any other background processes run by the test suite?

I can imagine gpg-agent is managed OK these days since it's started
automagically by gpg.

emacs seems to use the current process id in the socket name, so that
also should be OK, although it should maybe be replaced with something
more robust to avoid problems with pid rollover. I _think_ including the
test name in the emacs server would do the trick

The dtach socket is in the tmp.T* directory, so that should be OK.

I wonder if a good solution would be to make running the test suite in
parallel be opt-in (e.g. by configuration option). Or at least have a
way to disable it for situations like CI and autobuilders.

d

Re: [PATCH 1/3] tests: remove some redundant pre-cleanup of the corpus MAIL_DIR

[Tomi Ollila]

On Sat, May 04 2019, Jameson Rollins wrote:

This change LGTM, marked trivial, removed needs-review

> From: Jameson Graef Rollins <jrollins@finestructure.net>
>
> add_email_corpus itself does an rm -rf $MAIL_DIR, so these are not necessary.
> ---
>  test/T100-search-by-folder.sh | 1 -
>  test/T650-regexp-query.sh     | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/test/T100-search-by-folder.sh b/test/T100-search-by-folder.sh
> index a090f3d2..409cfdcc 100755
> --- a/test/T100-search-by-folder.sh
> +++ b/test/T100-search-by-folder.sh
> @@ -56,7 +56,6 @@ output=$(notmuch search folder:duplicate/bad/olds | notmuch_search_sanitize)
>  test_expect_equal "$output" "thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Bears (inbox unread)"
>  
>  # folder: and path: searches with full corpus
> -rm -rf $MAIL_DIR
>  add_email_corpus
>  
>  # add some more dupes
> diff --git a/test/T650-regexp-query.sh b/test/T650-regexp-query.sh
> index 92334ba0..43af3b47 100755
> --- a/test/T650-regexp-query.sh
> +++ b/test/T650-regexp-query.sh
> @@ -47,7 +47,6 @@ output=$(notmuch search 'path:/^bad$/' | notmuch_search_sanitize)
>  test_expect_equal "$output" "thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; To the bone (inbox unread)"
>  
>  # Use "standard" corpus from here on.
> -rm -rf $MAIL_DIR
>  add_email_corpus
>  
>  notmuch search --output=messages from:cworth > cworth.msg-ids
> -- 
> 2.20.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: parallelize test suite

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

On Sat 2019-05-04 19:53:23 -0300, David Bremner wrote:
> Last time we discussed parallel test running, there we concerns about
> multiple versions of certain servers colliding with each other. This
> still seems to be at least a theoretical issue with smtp-dummy, although
> a glance suggests that it might only currently be used in T310-emacs.sh.
> I'm not sure what a robust solution is here. 
>
> - gpg-agent
> - emacs
> - dtach
>
> Did I miss any other background processes run by the test suite?
>
> I can imagine gpg-agent is managed OK these days since it's started
> automagically by gpg.

gpg-agent is fine, because it's isolated by $GNUPGHOME, and each test
uses a distinct $GNUPGHOME (see GNUPGHOME="${TEST_TMPDIR}/gnupg in
test-lib.sh)

> emacs seems to use the current process id in the socket name, so that
> also should be OK, although it should maybe be replaced with something
> more robust to avoid problems with pid rollover. I _think_ including the
> test name in the emacs server would do the trick

I would have no objection to this improvement in isolation of the emacs
server processes, but i think the pid rollover race condition is so
minor that i don't think it sholud be a blocker for the adoption of this
series.

> The dtach socket is in the tmp.T* directory, so that should be OK.
>
> I wonder if a good solution would be to make running the test suite in
> parallel be opt-in (e.g. by configuration option). Or at least have a
> way to disable it for situations like CI and autobuilders.

I agree that making it possible to force serialized tests would be good.

I'd prefer that running tests in parallel be the default, though i
wouldn't object to a ./configure --serialize-tests option if someone
wants to implement it.

I actually think that CI and autobuilders *should* exercise the parallel
tests, as annoying as that might be initially, because it seems likely
to catch any other potential entanglements.

thanks for the review!

   --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: parallelize test suite

[Tomi Ollila]

On Sun, May 05 2019, Daniel Kahn Gillmor wrote:

> On Sat 2019-05-04 19:53:23 -0300, David Bremner wrote:
>> Last time we discussed parallel test running, there we concerns about
>> multiple versions of certain servers colliding with each other. This
>> still seems to be at least a theoretical issue with smtp-dummy, although
>> a glance suggests that it might only currently be used in T310-emacs.sh.

we,d need to change 25025 with something else -- configured in the test
script ? -- same could be used as part of emacs server socket ?


>> I'm not sure what a robust solution is here. 
>>
>> - gpg-agent
>> - emacs
>> - dtach
>>
>> Did I miss any other background processes run by the test suite?
>>
>> I can imagine gpg-agent is managed OK these days since it's started
>> automagically by gpg.
>
> gpg-agent is fine, because it's isolated by $GNUPGHOME, and each test
> uses a distinct $GNUPGHOME (see GNUPGHOME="${TEST_TMPDIR}/gnupg in
> test-lib.sh)
>
>> emacs seems to use the current process id in the socket name, so that
>> also should be OK, although it should maybe be replaced with something
>> more robust to avoid problems with pid rollover. I _think_ including the
>> test name in the emacs server would do the trick
>
> I would have no objection to this improvement in isolation of the emacs
> server processes, but i think the pid rollover race condition is so
> minor that i don't think it sholud be a blocker for the adoption of this
> series.

when test code done correctly (test_done ends it), the shell launching
emacs holds the pid $$ until emacs exits, so the pid is not reused
in name for any emacs process (but if bash died leaving emacs running
then we'd have chance for collisions...)

>
>> The dtach socket is in the tmp.T* directory, so that should be OK.
>>
>> I wonder if a good solution would be to make running the test suite in
>> parallel be opt-in (e.g. by configuration option). Or at least have a
>> way to disable it for situations like CI and autobuilders.
>
> I agree that making it possible to force serialized tests would be good.
>
> I'd prefer that running tests in parallel be the default, though i
> wouldn't object to a ./configure --serialize-tests option if someone
> wants to implement it.

no need for configure option, one can just remove parallel(1) from system ;)
... well, we could use ${PARALLEL:=parallel} in script for users to shadow
it out in some other way...

> I actually think that CI and autobuilders *should* exercise the parallel
> tests, as annoying as that might be initially, because it seems likely
> to catch any other potential entanglements.

... after it has been proven a bit more to work...

One thing more, the "perverse" pre-caching done in add_email_corpus
can be retained -- just do that step *before* going to parallelism...

>
> thanks for the review!
>
>    --dkg

Tomi

[PATCH] tests: environment variable to specify that tests should be serialized

[Rollins, Jameson]

From: Jameson Graef Rollins <jrollins@finestructure.net>

If NOTMUCH_TEST_SERIALIZE is non-null all tests will be run in series,
rather than in parallel.
---
 test/README       | 8 ++++++--
 test/notmuch-test | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/test/README b/test/README
index b378c3ff..3f54af58 100644
--- a/test/README
+++ b/test/README
@@ -43,7 +43,6 @@ these system tools. Most often the tests are written, reviewed and
 tested on Linux system so such portability issues arise from time to
 time.
 
-
 Running Tests
 -------------
 The easiest way to run tests is to say "make test", (or simply run the
@@ -105,9 +104,14 @@ to with emacs, e.g.
 
      make test TEST_CC=gcc TEST_CFLAGS="-g -O2"
 
+Parallel Execution
+------------------
+If either the moreutils or GNU "parallel" utility is available all
+tests will be run in parallel.  If the NOTMUCH_TEST_SERIALIZE variable
+is non-null all tests will be executed sequentially.
+
 Quiet Execution
 ---------------
-
 Normally, when new script starts and when test PASSes you get a message
 printed on screen. This printing can be disabled by setting the
 NOTMUCH_TEST_QUIET variable to a non-null value. Message on test
diff --git a/test/notmuch-test b/test/notmuch-test
index a4b7a1eb..bd3e080a 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -40,7 +40,7 @@ fi
 
 trap 'e=$?; kill $!; exit $e' HUP INT TERM
 # Run the tests
-if command -v parallel >/dev/null ; then
+if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
     if parallel -h | grep -q GNU ; then
         echo "INFO: running tests with GNU parallel"
         printf '%s\n' $TESTS | $TEST_TIMEOUT_CMD parallel
-- 
2.20.1

[PATCH] test: add configurable port to smtp-dummy

[David Bremner]

This is to allow smtp-dummy to potentially be used from multiple T*.sh
without collisions during parallel test running.
---
 test/T310-emacs.sh | 3 +++
 test/smtp-dummy.c  | 7 ++++++-
 test/test-lib.sh   | 4 +++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
index c06a8133..3eab1ba9 100755
--- a/test/T310-emacs.sh
+++ b/test/T310-emacs.sh
@@ -5,6 +5,9 @@ test_description="emacs interface"
 
 EXPECTED=$NOTMUCH_SRCDIR/test/emacs.expected-output
 
+# this port must be unique to each test script using smtp-dummy
+export SMTP_DUMMY_PORT=25125
+
 add_email_corpus
 
 # syntax errors in test-lib.el cause mysterious failures
diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
index 71992edd..4cd66f5c 100644
--- a/test/smtp-dummy.c
+++ b/test/smtp-dummy.c
@@ -120,6 +120,7 @@ int
 main (int argc, char *argv[])
 {
     const char *progname;
+    const char *port_str;
     char *output_filename;
     FILE *peer_file = NULL, *output = NULL;
     int sock = -1, peer, err;
@@ -191,7 +192,11 @@ main (int argc, char *argv[])
 
     memset (&addr, 0, sizeof (addr));
     addr.sin_family = AF_INET;
-    addr.sin_port = htons (25025);
+    if ((port_str = getenv ("SMTP_DUMMY_PORT")))
+	addr.sin_port = htons (atoi (port_str));
+    else
+	addr.sin_port = htons (25025);
+
     addr.sin_addr = *(struct in_addr *) hostinfo->h_addr;
     err = bind (sock, (struct sockaddr *) &addr, sizeof (addr));
     if (err) {
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 43339902..f3be537b 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -93,6 +93,8 @@ TEST_GDB=${TEST_GDB:-gdb}
 TEST_CC=${TEST_CC:-cc}
 TEST_CFLAGS=${TEST_CFLAGS:-"-g -O0"}
 
+SMTP_DUMMY_PORT=${SMTP_DUMMY_PORT:-25025}
+
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
 unset CDPATH
@@ -324,7 +326,7 @@ emacs_deliver_message ()
 	"(let ((message-send-mail-function 'message-smtpmail-send-it)
                (mail-host-address \"example.com\")
 	       (smtpmail-smtp-server \"localhost\")
-	       (smtpmail-smtp-service \"25025\"))
+	       (smtpmail-smtp-service \"${SMTP_DUMMY_PORT}\"))
 	   (notmuch-mua-mail)
 	   (message-goto-to)
 	   (insert \"test_suite@notmuchmail.org\nDate: 01 Jan 2000 12:00:00 -0000\")
-- 
2.20.1

Re: [PATCH] tests: environment variable to specify that tests should be serialized

[Tomi Ollila]

On Mon, May 06 2019, Jameson Rollins wrote:

> From: Jameson Graef Rollins <jrollins@finestructure.net>
>
> If NOTMUCH_TEST_SERIALIZE is non-null all tests will be run in series,
> rather than in parallel.

While I like this parallelization option, and hope a version (could be even 
David's smtp_dummy change) of it could be available in notmuch repository
as soon as possible, I would not like it being default -- just like make -J
is not default...

... it being default, unsuspicious user running `make test` might have his
multitasking maching eating too much resources for a particular purpose and
slowing everything else. 

The simplest way to invoke parallelized tests could be make test-parallel
and/or make test p=1 ...or NOTMUCH_TEST_PARALLELIZE make test.

That's my opinion written out loud... vote me over if disagreed.. :D

Tomi

> ---
>  test/README       | 8 ++++++--
>  test/notmuch-test | 2 +-
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/test/README b/test/README
> index b378c3ff..3f54af58 100644
> --- a/test/README
> +++ b/test/README
> @@ -43,7 +43,6 @@ these system tools. Most often the tests are written, reviewed and
>  tested on Linux system so such portability issues arise from time to
>  time.
>  
> -
>  Running Tests
>  -------------
>  The easiest way to run tests is to say "make test", (or simply run the
> @@ -105,9 +104,14 @@ to with emacs, e.g.
>  
>       make test TEST_CC=gcc TEST_CFLAGS="-g -O2"
>  
> +Parallel Execution
> +------------------
> +If either the moreutils or GNU "parallel" utility is available all
> +tests will be run in parallel.  If the NOTMUCH_TEST_SERIALIZE variable
> +is non-null all tests will be executed sequentially.
> +
>  Quiet Execution
>  ---------------
> -
>  Normally, when new script starts and when test PASSes you get a message
>  printed on screen. This printing can be disabled by setting the
>  NOTMUCH_TEST_QUIET variable to a non-null value. Message on test
> diff --git a/test/notmuch-test b/test/notmuch-test
> index a4b7a1eb..bd3e080a 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -40,7 +40,7 @@ fi
>  
>  trap 'e=$?; kill $!; exit $e' HUP INT TERM
>  # Run the tests
> -if command -v parallel >/dev/null ; then
> +if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
>      if parallel -h | grep -q GNU ; then
>          echo "INFO: running tests with GNU parallel"
>          printf '%s\n' $TESTS | $TEST_TIMEOUT_CMD parallel
> -- 
> 2.20.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] test: add configurable port to smtp-dummy

[Tomi Ollila]

On Mon, May 06 2019, David Bremner wrote:

> This is to allow smtp-dummy to potentially be used from multiple T*.sh
> without collisions during parallel test running.
> ---
>  test/T310-emacs.sh | 3 +++
>  test/smtp-dummy.c  | 7 ++++++-
>  test/test-lib.sh   | 4 +++-
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
> index c06a8133..3eab1ba9 100755
> --- a/test/T310-emacs.sh
> +++ b/test/T310-emacs.sh
> @@ -5,6 +5,9 @@ test_description="emacs interface"
>  
>  EXPECTED=$NOTMUCH_SRCDIR/test/emacs.expected-output
>  
> +# this port must be unique to each test script using smtp-dummy
> +export SMTP_DUMMY_PORT=25125

I suggest 25310 ! :D

(
    SMTP_DUMMY_PORT=${0%%-*}
    SMTP_DUMMY_PORT=$(( 25000 + ${SMTP_DUMMY_PORT#?} ))

is probably too complicated and error prone ;)

> +
>  add_email_corpus
>  
>  # syntax errors in test-lib.el cause mysterious failures
> diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
> index 71992edd..4cd66f5c 100644
> --- a/test/smtp-dummy.c
> +++ b/test/smtp-dummy.c
> @@ -120,6 +120,7 @@ int
>  main (int argc, char *argv[])
>  {
>      const char *progname;
> +    const char *port_str;
>      char *output_filename;
>      FILE *peer_file = NULL, *output = NULL;
>      int sock = -1, peer, err;
> @@ -191,7 +192,11 @@ main (int argc, char *argv[])
>  
>      memset (&addr, 0, sizeof (addr));
>      addr.sin_family = AF_INET;
> -    addr.sin_port = htons (25025);
> +    if ((port_str = getenv ("SMTP_DUMMY_PORT")))

If we required SMTP_DUMMY_PORT to be defined in environmment (and not
fallback to 25025 if not) we'd have less change to forget defining
it... but that may be too much to ask... ;/

... continuing on same subject below ...

> +	addr.sin_port = htons (atoi (port_str));
> +    else
> +	addr.sin_port = htons (25025);
> +
>      addr.sin_addr = *(struct in_addr *) hostinfo->h_addr;
>      err = bind (sock, (struct sockaddr *) &addr, sizeof (addr));
>      if (err) {
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 43339902..f3be537b 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -93,6 +93,8 @@ TEST_GDB=${TEST_GDB:-gdb}
>  TEST_CC=${TEST_CC:-cc}
>  TEST_CFLAGS=${TEST_CFLAGS:-"-g -O0"}
>  
> +SMTP_DUMMY_PORT=${SMTP_DUMMY_PORT:-25025}

... in that case this would not be set here ( but since we don't have 
`set -u` set in our test env (I have some half-decade-old patches for that
laying around though) we'd have to check that in emacs_deliver_message)

well, perhaps chance having same port is very low, in 10 years there has
been only one file running using that...

... but now I remember something (else) I though doing half-decade ago:

we run smtp-dummy as:

   smtp_dummy_pid=
   eval `$TEST_DIRECTORY/smtp-dummy --background sent_message`

if process binds using port `0` system fill find available port for it...

... which smtp-dummy could return in a variable to be evaluated ...

... and that could replace all this what has been done here ...


but whatever -- and IMO we could have parallelization w/o this already
not *IF* it is not done by default (or even default but then someone(tm)
has to fix this some way in near future (I'd vote smtp-dummy choosing 
port :D, that cannot be too hard.


Tomi


> +
>  # Protect ourselves from common misconfiguration to export
>  # CDPATH into the environment
>  unset CDPATH
> @@ -324,7 +326,7 @@ emacs_deliver_message ()
>  	"(let ((message-send-mail-function 'message-smtpmail-send-it)
>                 (mail-host-address \"example.com\")
>  	       (smtpmail-smtp-server \"localhost\")
> -	       (smtpmail-smtp-service \"25025\"))
> +	       (smtpmail-smtp-service \"${SMTP_DUMMY_PORT}\"))
>  	   (notmuch-mua-mail)
>  	   (message-goto-to)
>  	   (insert \"test_suite@notmuchmail.org\nDate: 01 Jan 2000 12:00:00 -0000\")
> -- 

Re: [PATCH] test: add configurable port to smtp-dummy

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On Mon 2019-05-06 22:39:26 +0300, Tomi Ollila wrote:
> we run smtp-dummy as:
>
>    smtp_dummy_pid=
>    eval `$TEST_DIRECTORY/smtp-dummy --background sent_message`
>
> if process binds using port `0` system fill find available port for it...
>
> ... which smtp-dummy could return in a variable to be evaluated ...
>
> ... and that could replace all this what has been done here ...

i agree with Tomi that something like this (smtp-dummy asks for an
available port and reports it), is the ideal solution, in that it means
that two copies of the notmuch testsuite itself could even run
independently and avoid contending for system resources.

but i think Bremner's manually-set port is a completely reasonable first
step, and we can adopt the proposed optimization later.  LGTM.

      --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH] tests: environment variable to specify that tests should be serialized

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On Mon 2019-05-06 22:15:49 +0300, Tomi Ollila wrote:
> While I like this parallelization option, and hope a version (could be even 
> David's smtp_dummy change) of it could be available in notmuch repository
> as soon as possible, I would not like it being default -- just like make -J
> is not default...
>
> ... it being default, unsuspicious user running `make test` might have his
> multitasking maching eating too much resources for a particular purpose and
> slowing everything else. 

if we're voting, i vote in favor of parallel defaults wherever possible,
with a way to force serialization.  I'd much rather have my machine
finish the thing i just asked it to do in less time by default, than
wait around twiddling my thumbs while the machine has spare cycles.  The
context switch for my mind is way more expensive than the context
switches going on within the computer.

The change from 2 minutes to < 30 seconds is a game-changer in terms of
keeping me focused while i'm developing.  make it easy to do that!

        --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: parallelize test suite

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Sun 2019-05-05 19:44:01 +0300, Tomi Ollila wrote:
>> I actually think that CI and autobuilders *should* exercise the parallel
>> tests, as annoying as that might be initially, because it seems likely
>> to catch any other potential entanglements.
>
> ... after it has been proven a bit more to work...

i tend to think the other way around -- the way to prove it is to push
it into the CI pipelines on the development branch, rather than trying
to prove it on individual developer systems.

> One thing more, the "perverse" pre-caching done in add_email_corpus
> can be retained -- just do that step *before* going to parallelism...

You could do that, but i'm not sure how much you'd gain from it -- its
current implementation is all mixed up with the parallelized tests,
anyway.  If you wanted to propose a concrete re-optimization like this,
i'd be happy to review it, test it and report back on the speedup
gained.

thanks for thinking this through, Tomi!

     --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[PATCH] test: let the OS choose a port for smtp-dummy

[David Bremner]

This should avoid potential collisions if we start running multiple
smtp-dummy processes in parallel.
---
 test/smtp-dummy.c | 15 ++++++++++++++-
 test/test-lib.sh  |  2 +-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
index 71992edd..a7c1fe4f 100644
--- a/test/smtp-dummy.c
+++ b/test/smtp-dummy.c
@@ -129,6 +129,7 @@ main (int argc, char *argv[])
     int reuse;
     int background;
     int ret = 0;
+    socklen_t addrlen;
 
     progname = argv[0];
 
@@ -191,7 +192,7 @@ main (int argc, char *argv[])
 
     memset (&addr, 0, sizeof (addr));
     addr.sin_family = AF_INET;
-    addr.sin_port = htons (25025);
+    addr.sin_port = 0;
     addr.sin_addr = *(struct in_addr *) hostinfo->h_addr;
     err = bind (sock, (struct sockaddr *) &addr, sizeof (addr));
     if (err) {
@@ -202,6 +203,18 @@ main (int argc, char *argv[])
 	goto DONE;
     }
 
+    addrlen = sizeof (addr);
+    err = getsockname (sock, (struct sockaddr *) &addr, &addrlen);
+    if (err) {
+	fprintf (stderr, "Error: getsockname() failed: %s\n",
+		 strerror (errno));
+	close (sock);
+	ret = 1;
+	goto DONE;
+    }
+
+    printf ("smtp_dummy_port='%d'\n", ntohs (addr.sin_port));
+
     err = listen (sock, 1);
     if (err) {
 	fprintf (stderr, "Error: listen() failed: %s\n",
diff --git a/test/test-lib.sh b/test/test-lib.sh
index f5d367aa..507886ba 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -326,7 +326,7 @@ emacs_deliver_message ()
 	"(let ((message-send-mail-function 'message-smtpmail-send-it)
                (mail-host-address \"example.com\")
 	       (smtpmail-smtp-server \"localhost\")
-	       (smtpmail-smtp-service \"25025\"))
+	       (smtpmail-smtp-service \"${smtp_dummy_port}\"))
 	   (notmuch-mua-mail)
 	   (message-goto-to)
 	   (insert \"test_suite@notmuchmail.org\nDate: 01 Jan 2000 12:00:00 -0000\")
-- 
2.20.1

Re: parallelize test suite

[David Bremner]

"Rollins, Jameson" <jrollins@caltech.edu> writes:

> This is a simple patch series that will run the entire test suite in
> parallel if either the moreutils or GNU parallel utility is
> available.  On my 8-core machine the full test suite will now run in
> under 20 seconds, which is a pretty huge improvement.

I've taken the plunge and merged this series to master. I also posted an
update to smtp-dummy following Tomi's suggestion to bind to port 0.

d

Re: [PATCH] test: let the OS choose a port for smtp-dummy

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Tue 2019-05-07 07:20:49 -0300, David Bremner wrote:
> This should avoid potential collisions if we start running multiple
> smtp-dummy processes in parallel.

This is excellent, simple, and clearly the right thing to do.  I've
reviewed it, and am running it on my own development branch with no
problems.  Thanks, Bremner!

please merge.

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH] test: let the OS choose a port for smtp-dummy

[Tomi Ollila]

On Tue, May 07 2019, Daniel Kahn Gillmor wrote:

> On Tue 2019-05-07 07:20:49 -0300, David Bremner wrote:
>> This should avoid potential collisions if we start running multiple
>> smtp-dummy processes in parallel.
>
> This is excellent, simple, and clearly the right thing to do.  I've
> reviewed it, and am running it on my own development branch with no
> problems.  Thanks, Bremner!
>
> please merge.

i second that, please do!

Tomi


>
>        --dkg

Re: [PATCH] test: let the OS choose a port for smtp-dummy

[David Bremner]

David Bremner <david@tethera.net> writes:

> This should avoid potential collisions if we start running multiple
> smtp-dummy processes in parallel.

merged to master