unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH 0/9] test: ruby: several cleanups and simplifications
@ 2021-05-01 11:59 Felipe Contreras
  2021-05-01 11:59 ` [PATCH 1/9] test: move test_ruby() inside the only client Felipe Contreras
                   ` (9 more replies)
  0 siblings, 10 replies; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

I found a lot of areas of improvement in the Ruby tests, so I decided to clean them up.

With these changes the tests are now much simpler and follow more closely the typical Ruby idioms.

Felipe Contreras (9):
  test: move test_ruby() inside the only client
  test: ruby: refactor test_ruby()
  test: ruby: simplify MAIL_DIR check
  test: ruby: simplify MAIL_DIR initialization
  test: ruby: simplify test_ruby()
  test: ruby: use much more standard puts
  test: ruby: use much more standard Ruby idioms
  test: ruby: don't use instance variables
  test: ruby: simplify output comparison

 test/T395-ruby.sh | 94 +++++++++++++++--------------------------------
 test/test-lib.sh  |  4 --
 2 files changed, 30 insertions(+), 68 deletions(-)

-- 
2.31.0

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/9] test: move test_ruby() inside the only client
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 10:49   ` David Bremner
  2021-05-01 11:59 ` [PATCH 2/9] test: ruby: refactor test_ruby() Felipe Contreras
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Not much point in polluting the main library, and also will be useful to
modify it in tandem with the tests.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 4 ++++
 test/test-lib.sh  | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index a0b76eb8..fec1f5ef 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -8,6 +8,10 @@ fi
 
 add_email_corpus
 
+test_ruby() {
+    MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+}
+
 test_begin_subtest "compare thread ids"
 test_ruby <<"EOF"
 require 'notmuch'
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 4c9f2a21..ec0ba7f7 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1109,10 +1109,6 @@ test_python() {
 	$NOTMUCH_PYTHON -B - > OUTPUT
 }
 
-test_ruby() {
-    MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
-}
-
 test_C () {
     local exec_file test_file
     exec_file="test${test_count}"
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/9] test: ruby: refactor test_ruby()
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
  2021-05-01 11:59 ` [PATCH 1/9] test: move test_ruby() inside the only client Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 10:50   ` David Bremner
  2021-05-01 11:59 ` [PATCH 3/9] test: ruby: simplify MAIL_DIR check Felipe Contreras
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

There's no point in repeating the same initialization in all the tests.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 48 +++++++++++------------------------------------
 1 file changed, 11 insertions(+), 37 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index fec1f5ef..1d27e191 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -9,17 +9,21 @@ fi
 add_email_corpus
 
 test_ruby() {
-    MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+    (
+	cat <<-\EOF
+	require 'notmuch'
+	$maildir = ENV['MAIL_DIR']
+	if not $maildir then
+	  abort('environment variable MAIL_DIR must be set')
+	end
+	@db = Notmuch::Database.new($maildir)
+	EOF
+	cat
+    ) | MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
 }
 
 test_begin_subtest "compare thread ids"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for t in @q.search_threads do
@@ -31,12 +35,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "compare message ids"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for m in @q.search_messages do
@@ -48,12 +46,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get non-existent file"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 result = @db.find_message_by_filename('i-dont-exist')
 print (result == nil)
 EOF
@@ -61,12 +53,6 @@ test_expect_equal "$(cat OUTPUT)" "true"
 
 test_begin_subtest "count messages"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 print @q.count_messages(),"\n"
 EOF
@@ -75,12 +61,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "count threads"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @q = @db.query('tag:inbox')
 print @q.count_threads(),"\n"
 EOF
@@ -89,12 +69,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get all tags"
 test_ruby <<"EOF"
-require 'notmuch'
-$maildir = ENV['MAIL_DIR']
-if not $maildir then
-  abort('environment variable MAIL_DIR must be set')
-end
-@db = Notmuch::Database.new($maildir)
 @t = @db.all_tags()
 for tag in @t do
    print tag,"\n"
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/9] test: ruby: simplify MAIL_DIR check
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
  2021-05-01 11:59 ` [PATCH 1/9] test: move test_ruby() inside the only client Felipe Contreras
  2021-05-01 11:59 ` [PATCH 2/9] test: ruby: refactor test_ruby() Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 10:51   ` David Bremner
  2021-05-01 11:59 ` [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization Felipe Contreras
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 1d27e191..94fab106 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -12,10 +12,7 @@ test_ruby() {
     (
 	cat <<-\EOF
 	require 'notmuch'
-	$maildir = ENV['MAIL_DIR']
-	if not $maildir then
-	  abort('environment variable MAIL_DIR must be set')
-	end
+	$maildir = ENV['MAIL_DIR'] || abort('MAIL_DIR not set')
 	@db = Notmuch::Database.new($maildir)
 	EOF
 	cat
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (2 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 3/9] test: ruby: simplify MAIL_DIR check Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 10:56   ` David Bremner
  2021-05-01 11:59 ` [PATCH 5/9] test: ruby: simplify test_ruby() Felipe Contreras
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

There's no need to complicate the script passing the MAIL_DIR
environment variable.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 94fab106..67d6e205 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -10,13 +10,12 @@ add_email_corpus
 
 test_ruby() {
     (
-	cat <<-\EOF
+	cat <<-EOF
 	require 'notmuch'
-	$maildir = ENV['MAIL_DIR'] || abort('MAIL_DIR not set')
-	@db = Notmuch::Database.new($maildir)
+	@db = Notmuch::Database.new('$MAIL_DIR')
 	EOF
 	cat
-    ) | MAIL_DIR=$MAIL_DIR $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+    ) | $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
 }
 
 test_begin_subtest "compare thread ids"
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 5/9] test: ruby: simplify test_ruby()
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (3 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 11:02   ` David Bremner
  2021-05-01 11:59 ` [PATCH 6/9] test: ruby: use much more standard puts Felipe Contreras
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

We always do test_expect_equal_file, so do it in test_ruby() directly.

The only subtest where we don't (get non-existent file) can be easily
modified.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 67d6e205..55bf4c2b 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -16,9 +16,11 @@ test_ruby() {
 	EOF
 	cat
     ) | $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
+    test_expect_equal_file EXPECTED OUTPUT
 }
 
 test_begin_subtest "compare thread ids"
+notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
@@ -26,10 +28,9 @@ for t in @q.search_threads do
   print t.thread_id, "\n"
 end
 EOF
-notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "compare message ids"
+notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
@@ -37,40 +38,35 @@ for m in @q.search_messages do
   print m.message_id, "\n"
 end
 EOF
-notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get non-existent file"
+echo -n true > EXPECTED
 test_ruby <<"EOF"
 result = @db.find_message_by_filename('i-dont-exist')
 print (result == nil)
 EOF
-test_expect_equal "$(cat OUTPUT)" "true"
 
 test_begin_subtest "count messages"
+notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 print @q.count_messages(),"\n"
 EOF
-notmuch count --output=messages tag:inbox > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "count threads"
+notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 print @q.count_threads(),"\n"
 EOF
-notmuch count --output=threads tag:inbox > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "get all tags"
+notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
 @t = @db.all_tags()
 for tag in @t do
    print tag,"\n"
 end
 EOF
-notmuch search --output=tags '*' > EXPECTED
-test_expect_equal_file EXPECTED OUTPUT
 
 test_done
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 6/9] test: ruby: use much more standard puts
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (4 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 5/9] test: ruby: simplify test_ruby() Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 11:03   ` David Bremner
  2021-05-01 11:59 ` [PATCH 7/9] test: ruby: use much more standard Ruby idioms Felipe Contreras
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 55bf4c2b..f871ddd9 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -25,7 +25,7 @@ test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for t in @q.search_threads do
-  print t.thread_id, "\n"
+  puts t.thread_id
 end
 EOF
 
@@ -35,29 +35,29 @@ test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
 for m in @q.search_messages do
-  print m.message_id, "\n"
+  puts m.message_id
 end
 EOF
 
 test_begin_subtest "get non-existent file"
-echo -n true > EXPECTED
+echo true > EXPECTED
 test_ruby <<"EOF"
 result = @db.find_message_by_filename('i-dont-exist')
-print (result == nil)
+puts (result == nil)
 EOF
 
 test_begin_subtest "count messages"
 notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
-print @q.count_messages(),"\n"
+puts @q.count_messages()
 EOF
 
 test_begin_subtest "count threads"
 notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
-print @q.count_threads(),"\n"
+puts @q.count_threads()
 EOF
 
 test_begin_subtest "get all tags"
@@ -65,7 +65,7 @@ notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
 @t = @db.all_tags()
 for tag in @t do
-   print tag,"\n"
+   puts tag
 end
 EOF
 
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 7/9] test: ruby: use much more standard Ruby idioms
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (5 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 6/9] test: ruby: use much more standard puts Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 11:07   ` David Bremner
  2021-05-01 11:59 ` [PATCH 8/9] test: ruby: don't use instance variables Felipe Contreras
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index f871ddd9..f5a8d245 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -24,7 +24,7 @@ notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread://
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
-for t in @q.search_threads do
+@q.search_threads.each do |t|
   puts t.thread_id
 end
 EOF
@@ -34,38 +34,34 @@ notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// >
 test_ruby <<"EOF"
 @q = @db.query('tag:inbox')
 @q.sort = Notmuch::SORT_OLDEST_FIRST
-for m in @q.search_messages do
+@q.search_messages.each do |m|
   puts m.message_id
 end
 EOF
 
 test_begin_subtest "get non-existent file"
-echo true > EXPECTED
+echo nil > EXPECTED
 test_ruby <<"EOF"
-result = @db.find_message_by_filename('i-dont-exist')
-puts (result == nil)
+p @db.find_message_by_filename('i-dont-exist')
 EOF
 
 test_begin_subtest "count messages"
 notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-puts @q.count_messages()
+puts @db.query('tag:inbox').count_messages()
 EOF
 
 test_begin_subtest "count threads"
 notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-puts @q.count_threads()
+puts @db.query('tag:inbox').count_threads()
 EOF
 
 test_begin_subtest "get all tags"
 notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
-@t = @db.all_tags()
-for tag in @t do
-   puts tag
+@db.all_tags.each do |tag|
+  puts tag
 end
 EOF
 
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 8/9] test: ruby: don't use instance variables
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (6 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 7/9] test: ruby: use much more standard Ruby idioms Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 11:08   ` David Bremner
  2021-05-01 11:59 ` [PATCH 9/9] test: ruby: simplify output comparison Felipe Contreras
  2021-05-02 11:22 ` [PATCH 0/9] test: ruby: several cleanups and simplifications David Bremner
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Local variables are perfectly fine.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index f5a8d245..30168109 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -12,7 +12,7 @@ test_ruby() {
     (
 	cat <<-EOF
 	require 'notmuch'
-	@db = Notmuch::Database.new('$MAIL_DIR')
+	db = Notmuch::Database.new('$MAIL_DIR')
 	EOF
 	cat
     ) | $NOTMUCH_RUBY -I "$NOTMUCH_BUILDDIR/bindings/ruby"> OUTPUT
@@ -22,9 +22,9 @@ test_ruby() {
 test_begin_subtest "compare thread ids"
 notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-@q.sort = Notmuch::SORT_OLDEST_FIRST
-@q.search_threads.each do |t|
+q = db.query('tag:inbox')
+q.sort = Notmuch::SORT_OLDEST_FIRST
+q.search_threads.each do |t|
   puts t.thread_id
 end
 EOF
@@ -32,9 +32,9 @@ EOF
 test_begin_subtest "compare message ids"
 notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
 test_ruby <<"EOF"
-@q = @db.query('tag:inbox')
-@q.sort = Notmuch::SORT_OLDEST_FIRST
-@q.search_messages.each do |m|
+q = db.query('tag:inbox')
+q.sort = Notmuch::SORT_OLDEST_FIRST
+q.search_messages.each do |m|
   puts m.message_id
 end
 EOF
@@ -42,25 +42,25 @@ EOF
 test_begin_subtest "get non-existent file"
 echo nil > EXPECTED
 test_ruby <<"EOF"
-p @db.find_message_by_filename('i-dont-exist')
+p db.find_message_by_filename('i-dont-exist')
 EOF
 
 test_begin_subtest "count messages"
 notmuch count --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
-puts @db.query('tag:inbox').count_messages()
+puts db.query('tag:inbox').count_messages()
 EOF
 
 test_begin_subtest "count threads"
 notmuch count --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
-puts @db.query('tag:inbox').count_threads()
+puts db.query('tag:inbox').count_threads()
 EOF
 
 test_begin_subtest "get all tags"
 notmuch search --output=tags '*' > EXPECTED
 test_ruby <<"EOF"
-@db.all_tags.each do |tag|
+db.all_tags.each do |tag|
   puts tag
 end
 EOF
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 9/9] test: ruby: simplify output comparison
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (7 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 8/9] test: ruby: don't use instance variables Felipe Contreras
@ 2021-05-01 11:59 ` Felipe Contreras
  2021-05-02 11:09   ` David Bremner
  2021-05-02 11:22 ` [PATCH 0/9] test: ruby: several cleanups and simplifications David Bremner
  9 siblings, 1 reply; 26+ messages in thread
From: Felipe Contreras @ 2021-05-01 11:59 UTC (permalink / raw)
  To: notmuch; +Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T395-ruby.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
index 30168109..597330d3 100755
--- a/test/T395-ruby.sh
+++ b/test/T395-ruby.sh
@@ -20,22 +20,22 @@ test_ruby() {
 }
 
 test_begin_subtest "compare thread ids"
-notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread:// > EXPECTED
+notmuch search --sort=oldest-first --output=threads tag:inbox > EXPECTED
 test_ruby <<"EOF"
 q = db.query('tag:inbox')
 q.sort = Notmuch::SORT_OLDEST_FIRST
 q.search_threads.each do |t|
-  puts t.thread_id
+  puts 'thread:%s' % t.thread_id
 end
 EOF
 
 test_begin_subtest "compare message ids"
-notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
+notmuch search --sort=oldest-first --output=messages tag:inbox > EXPECTED
 test_ruby <<"EOF"
 q = db.query('tag:inbox')
 q.sort = Notmuch::SORT_OLDEST_FIRST
 q.search_messages.each do |m|
-  puts m.message_id
+  puts 'id:%s' % m.message_id
 end
 EOF
 
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/9] test: move test_ruby() inside the only client
  2021-05-01 11:59 ` [PATCH 1/9] test: move test_ruby() inside the only client Felipe Contreras
@ 2021-05-02 10:49   ` David Bremner
  2021-05-02 18:28     ` Tomi Ollila
  0 siblings, 1 reply; 26+ messages in thread
From: David Bremner @ 2021-05-02 10:49 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Not much point in polluting the main library, and also will be useful to
> modify it in tandem with the tests.
>

I can live with this change.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/9] test: ruby: refactor test_ruby()
  2021-05-01 11:59 ` [PATCH 2/9] test: ruby: refactor test_ruby() Felipe Contreras
@ 2021-05-02 10:50   ` David Bremner
  0 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 10:50 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no point in repeating the same initialization in all the tests.
>

LGTM

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/9] test: ruby: simplify MAIL_DIR check
  2021-05-01 11:59 ` [PATCH 3/9] test: ruby: simplify MAIL_DIR check Felipe Contreras
@ 2021-05-02 10:51   ` David Bremner
  0 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 10:51 UTC (permalink / raw)
  To: Felipe Contreras, notmuch


LGTM. Although I am no expert on ruby idiom.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization
  2021-05-01 11:59 ` [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization Felipe Contreras
@ 2021-05-02 10:56   ` David Bremner
  0 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 10:56 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no need to complicate the script passing the MAIL_DIR
> environment variable.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

The interpolation inside single quotes is slightly surprising, but OK

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/9] test: ruby: simplify test_ruby()
  2021-05-01 11:59 ` [PATCH 5/9] test: ruby: simplify test_ruby() Felipe Contreras
@ 2021-05-02 11:02   ` David Bremner
  2021-05-02 21:53     ` Felipe Contreras
  0 siblings, 1 reply; 26+ messages in thread
From: David Bremner @ 2021-05-02 11:02 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We always do test_expect_equal_file, so do it in test_ruby() directly.
>
> The only subtest where we don't (get non-existent file) can be easily
> modified.

I'm slightly hesitent since every other test ends with test_expect_*.
OTOH it is self contained within this one file so I guess I can live
with this change.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/9] test: ruby: use much more standard puts
  2021-05-01 11:59 ` [PATCH 6/9] test: ruby: use much more standard puts Felipe Contreras
@ 2021-05-02 11:03   ` David Bremner
  0 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 11:03 UTC (permalink / raw)
  To: Felipe Contreras, notmuch


No objection.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms
  2021-05-01 11:59 ` [PATCH 7/9] test: ruby: use much more standard Ruby idioms Felipe Contreras
@ 2021-05-02 11:07   ` David Bremner
  2021-05-02 19:00     ` Tomi Ollila
  2021-05-02 22:11     ` Felipe Contreras
  0 siblings, 2 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 11:07 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  test/T395-ruby.sh | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
> index f871ddd9..f5a8d245 100755
> --- a/test/T395-ruby.sh
> +++ b/test/T395-ruby.sh
> @@ -24,7 +24,7 @@ notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread://
>  test_ruby <<"EOF"
>  @q = @db.query('tag:inbox')
>  @q.sort = Notmuch::SORT_OLDEST_FIRST
> -for t in @q.search_threads do
> +@q.search_threads.each do |t|
>    puts t.thread_id
>  end
>  EOF

The downside to these changes is that they make the tests harder for the
non-rubyist (i.e. me) to read. So I'm not (yet) convinced this is a good change.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 8/9] test: ruby: don't use instance variables
  2021-05-01 11:59 ` [PATCH 8/9] test: ruby: don't use instance variables Felipe Contreras
@ 2021-05-02 11:08   ` David Bremner
  0 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 11:08 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Local variables are perfectly fine.
>

LGTM (given it works)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 9/9] test: ruby: simplify output comparison
  2021-05-01 11:59 ` [PATCH 9/9] test: ruby: simplify output comparison Felipe Contreras
@ 2021-05-02 11:09   ` David Bremner
  0 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 11:09 UTC (permalink / raw)
  To: Felipe Contreras, notmuch


I can live with this change.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/9] test: ruby: several cleanups and simplifications
  2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
                   ` (8 preceding siblings ...)
  2021-05-01 11:59 ` [PATCH 9/9] test: ruby: simplify output comparison Felipe Contreras
@ 2021-05-02 11:22 ` David Bremner
  9 siblings, 0 replies; 26+ messages in thread
From: David Bremner @ 2021-05-02 11:22 UTC (permalink / raw)
  To: Felipe Contreras, notmuch
  Cc: Daniel Kahn Gillmor, Ludovic LANGE, Tomi Ollila,
	Stefano Zacchiroli

Felipe Contreras <felipe.contreras@gmail.com> writes:

> I found a lot of areas of improvement in the Ruby tests, so I decided to clean them up.
>
> With these changes the tests are now much simpler and follow more closely the typical Ruby idioms.
>
> Felipe Contreras (9):
>   test: move test_ruby() inside the only client
>   test: ruby: refactor test_ruby()
>   test: ruby: simplify MAIL_DIR check
>   test: ruby: simplify MAIL_DIR initialization
>   test: ruby: simplify test_ruby()
>   test: ruby: use much more standard puts

I have applied the first 6 patches to master.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/9] test: move test_ruby() inside the only client
  2021-05-02 10:49   ` David Bremner
@ 2021-05-02 18:28     ` Tomi Ollila
  0 siblings, 0 replies; 26+ messages in thread
From: Tomi Ollila @ 2021-05-02 18:28 UTC (permalink / raw)
  To: David Bremner, Felipe Contreras, notmuch

On Sun, May 02 2021, David Bremner wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Not much point in polluting the main library, and also will be useful to
>> modify it in tandem with the tests.
>>
>
> I can live with this change.

I'd say strong LGTM :D

Tomi

>
> d
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms
  2021-05-02 11:07   ` David Bremner
@ 2021-05-02 19:00     ` Tomi Ollila
  2021-05-05 22:29       ` David Bremner
  2021-05-02 22:11     ` Felipe Contreras
  1 sibling, 1 reply; 26+ messages in thread
From: Tomi Ollila @ 2021-05-02 19:00 UTC (permalink / raw)
  To: David Bremner, Felipe Contreras, notmuch

On Sun, May 02 2021, David Bremner wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  test/T395-ruby.sh | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/test/T395-ruby.sh b/test/T395-ruby.sh
>> index f871ddd9..f5a8d245 100755
>> --- a/test/T395-ruby.sh
>> +++ b/test/T395-ruby.sh
>> @@ -24,7 +24,7 @@ notmuch search --sort=oldest-first --output=threads tag:inbox | sed s/^thread://
>>  test_ruby <<"EOF"
>>  @q = @db.query('tag:inbox')
>>  @q.sort = Notmuch::SORT_OLDEST_FIRST
>> -for t in @q.search_threads do
>> +@q.search_threads.each do |t|
>>    puts t.thread_id
>>  end
>>  EOF
>
> The downside to these changes is that they make the tests harder for the
> non-rubyist (i.e. me) to read. So I'm not (yet) convinced this is a good
> change.

I am convinced that this is good change (like all the other changes
in this series). 

Every now and then I encounter ruby code, and have seen syntax like
`@q.search_threads.each do |t|` been used, so more exposure to that
syntax is good thing (for everyone (IMO))

Tomi

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/9] test: ruby: simplify test_ruby()
  2021-05-02 11:02   ` David Bremner
@ 2021-05-02 21:53     ` Felipe Contreras
  0 siblings, 0 replies; 26+ messages in thread
From: Felipe Contreras @ 2021-05-02 21:53 UTC (permalink / raw)
  To: David Bremner; +Cc: notmuch@notmuchmail.org

On Sun, May 2, 2021 at 6:02 AM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We always do test_expect_equal_file, so do it in test_ruby() directly.
> >
> > The only subtest where we don't (get non-existent file) can be easily
> > modified.
>
> I'm slightly hesitent since every other test ends with test_expect_*.
> OTOH it is self contained within this one file so I guess I can live
> with this change.

The test could be named test_ruby_expect and be passed the EXPECTED
file, or something.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms
  2021-05-02 11:07   ` David Bremner
  2021-05-02 19:00     ` Tomi Ollila
@ 2021-05-02 22:11     ` Felipe Contreras
  1 sibling, 0 replies; 26+ messages in thread
From: Felipe Contreras @ 2021-05-02 22:11 UTC (permalink / raw)
  To: David Bremner; +Cc: notmuch@notmuchmail.org

On Sun, May 2, 2021 at 6:07 AM David Bremner <david@tethera.net> wrote:

> The downside to these changes is that they make the tests harder for the
> non-rubyist (i.e. me) to read. So I'm not (yet) convinced this is a good change.

Yeah, but what about the rubyists? I have *never* seen the `for`
statement used anywhere. It's basically the first thing you learn in
Ruby, right after variables and objects[1].

  File.open('README').each { |l| puts l }

It also matches the notmuch_rb_threads_each () C function, which BTW
should return an Enumerator when no block is given, in order to match
other `each` methods:

    File.open('README').each # this returns an Enumerator

Being more familiar with Ruby can only improve the Ruby bindings.

Cheers.

[1] http://docs.ruby-doc.com/docs/ProgrammingRuby/

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms
  2021-05-02 19:00     ` Tomi Ollila
@ 2021-05-05 22:29       ` David Bremner
  2021-05-06 11:52         ` Felipe Contreras
  0 siblings, 1 reply; 26+ messages in thread
From: David Bremner @ 2021-05-05 22:29 UTC (permalink / raw)
  To: Tomi Ollila, Felipe Contreras, notmuch

Tomi Ollila <tomi.ollila@iki.fi> writes:
>
> I am convinced that this is good change (like all the other changes
> in this series). 
>
> Every now and then I encounter ruby code, and have seen syntax like
> `@q.search_threads.each do |t|` been used, so more exposure to that
> syntax is good thing (for everyone (IMO))
>

OK, I've applied the patch (and the others in the series). I have my
(already expressed) reservations, but I don't want to stand in the way
of progress on the ruby bindings.

d

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] test: ruby: use much more standard Ruby idioms
  2021-05-05 22:29       ` David Bremner
@ 2021-05-06 11:52         ` Felipe Contreras
  0 siblings, 0 replies; 26+ messages in thread
From: Felipe Contreras @ 2021-05-06 11:52 UTC (permalink / raw)
  To: David Bremner; +Cc: Tomi Ollila, notmuch@notmuchmail.org

On Wed, May 5, 2021 at 5:29 PM David Bremner <david@tethera.net> wrote:
> Tomi Ollila <tomi.ollila@iki.fi> writes:
> >
> > I am convinced that this is good change (like all the other changes
> > in this series).
> >
> > Every now and then I encounter ruby code, and have seen syntax like
> > `@q.search_threads.each do |t|` been used, so more exposure to that
> > syntax is good thing (for everyone (IMO))
>
> OK, I've applied the patch (and the others in the series). I have my
> (already expressed) reservations, but I don't want to stand in the way
> of progress on the ruby bindings.

If you have reservations about the code, feel free to let me know and
I would try to address them in further rerolls of the patch series.

I'm a long term git contributor, so I'm used to several rounds of
reviews, a bit of pushback isn't going to deter me.

But at least the patches that you pushed I don't think could have any issues.

Let's see the rest.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-05-06 11:53 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 11:59 [PATCH 0/9] test: ruby: several cleanups and simplifications Felipe Contreras
2021-05-01 11:59 ` [PATCH 1/9] test: move test_ruby() inside the only client Felipe Contreras
2021-05-02 10:49   ` David Bremner
2021-05-02 18:28     ` Tomi Ollila
2021-05-01 11:59 ` [PATCH 2/9] test: ruby: refactor test_ruby() Felipe Contreras
2021-05-02 10:50   ` David Bremner
2021-05-01 11:59 ` [PATCH 3/9] test: ruby: simplify MAIL_DIR check Felipe Contreras
2021-05-02 10:51   ` David Bremner
2021-05-01 11:59 ` [PATCH 4/9] test: ruby: simplify MAIL_DIR initialization Felipe Contreras
2021-05-02 10:56   ` David Bremner
2021-05-01 11:59 ` [PATCH 5/9] test: ruby: simplify test_ruby() Felipe Contreras
2021-05-02 11:02   ` David Bremner
2021-05-02 21:53     ` Felipe Contreras
2021-05-01 11:59 ` [PATCH 6/9] test: ruby: use much more standard puts Felipe Contreras
2021-05-02 11:03   ` David Bremner
2021-05-01 11:59 ` [PATCH 7/9] test: ruby: use much more standard Ruby idioms Felipe Contreras
2021-05-02 11:07   ` David Bremner
2021-05-02 19:00     ` Tomi Ollila
2021-05-05 22:29       ` David Bremner
2021-05-06 11:52         ` Felipe Contreras
2021-05-02 22:11     ` Felipe Contreras
2021-05-01 11:59 ` [PATCH 8/9] test: ruby: don't use instance variables Felipe Contreras
2021-05-02 11:08   ` David Bremner
2021-05-01 11:59 ` [PATCH 9/9] test: ruby: simplify output comparison Felipe Contreras
2021-05-02 11:09   ` David Bremner
2021-05-02 11:22 ` [PATCH 0/9] test: ruby: several cleanups and simplifications David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).