From 895d8403cdd6f4219f0ca2153b9f813567ea26cf Mon Sep 17 00:00:00 2001 From: nick evans Date: Sun, 5 May 2024 13:51:00 -0400 Subject: [PATCH 1/2] Extract public quit!/disconnect from do_finish This renames `#do_finish` to `#quit!`, makes it public, adds an option to convert exceptions into warnings, and extracts the contents of the ensure block into a new public `#disconnect` method. The motivation for making these public is given in the rdoc. As documented in the rdoc, `#quit!`: > Calls #quit and ensures that #disconnect is called. Returns the > result from #quit. Returns +nil+ when the client is already > disconnected or when a prior error prevents the client from calling > #quit. Unlike #finish, this an exception will not be raised when the > client has not started. > > If #quit raises a StandardError, the connection is dropped and the > exception is re-raised. When exception: :warn is specified, > a warning is printed and the exception is returned. When > exception: false is specified, the warning is not printed. > This is useful when the connection must be dropped, for example in a > test suite or due to security concerns. As documented in the rdoc, `#disconnect`: > Disconnects the socket without checking if the connection has started > yet, and without sending a final QUIT message to the server. > > Generally, either #finish or #quit! should be used instead. fix docs fix doc --- lib/net/smtp.rb | 38 ++++++++++++++++++++-- test/net/smtp/test_smtp.rb | 66 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/lib/net/smtp.rb b/lib/net/smtp.rb index cca06e6..215b41a 100644 --- a/lib/net/smtp.rb +++ b/lib/net/smtp.rb @@ -642,7 +642,7 @@ def start(*args, helo: nil, user: nil, secret: nil, password: nil, authtype: nil do_start helo, user, secret, authtype return yield(self) ensure - do_finish + quit! end else do_start helo, user, secret, authtype @@ -654,7 +654,7 @@ def start(*args, helo: nil, user: nil, secret: nil, password: nil, authtype: nil # Raises IOError if not started. def finish raise IOError, 'not yet started' unless started? - do_finish + quit! end private @@ -728,15 +728,47 @@ def do_helo(helo_domain) raise end - def do_finish + public + + # Calls #quit and ensures that #disconnect is called. Returns the result + # from #quit. Returns +nil+ when the client is already disconnected or when + # a prior error prevents the client from calling #quit. Unlike #finish, an + # exception will not be raised when the client has not started. + # + # If #quit raises a StandardError, the connection is dropped and the + # exception is re-raised. When exception: :warn is specified, a + # warning is printed and the exception is returned. When exception: + # false is specified, the warning is not printed. This is useful when + # the connection must be dropped, for example in a test suite or due to + # security concerns. + # + # Related: #finish, #quit, #disconnect + def quit!(exception: true) quit if @socket and not @socket.closed? and not @error_occurred + rescue => ex + if exception == :warn + warn "%s during %p #%s: %s" % [ex.class, self, __method__, ex] + elsif exception + raise ex + end + ex ensure + disconnect + end + + # Disconnects the socket without checking if the connection has started yet, + # and without sending a final QUIT message to the server. + # + # Generally, either #finish or #quit! should be used instead. + def disconnect @started = false @error_occurred = false @socket.close if @socket @socket = nil end + private + def requires_smtputf8(address) if address.kind_of? Address !address.address.ascii_only? diff --git a/test/net/smtp/test_smtp.rb b/test/net/smtp/test_smtp.rb index 3b9e245..004c303 100644 --- a/test/net/smtp/test_smtp.rb +++ b/test/net/smtp/test_smtp.rb @@ -594,6 +594,72 @@ def test_mailfrom_with_smtputf_detection assert_equal "MAIL FROM: SMTPUTF8\r\n", server.commands.last end + def test_quit + server = FakeServer.start + smtp = Net::SMTP.start 'localhost', server.port + smtp.quit + assert_equal "QUIT\r\n", server.commands.last + + # Already finished: + assert_raise Errno, IOError, EOFError do + smtp.quit + end + + server = FakeServer.start + def server.quit + @sock.puts "400 BUSY\r\n" + end + smtp = Net::SMTP.start 'localhost', server.port + assert_raise Net::SMTPServerBusy do + smtp.quit + end + assert_equal "QUIT\r\n", server.commands.last + assert smtp.started? + end + + def test_quit! + server = FakeServer.start + smtp = Net::SMTP.start 'localhost', server.port + smtp.quit! + assert_equal "QUIT\r\n", server.commands.last + refute smtp.started? + + # Already finished: + smtp.quit! + end + + def test_quit_and_reraise + server = FakeServer.start + def server.quit + @sock.puts "400 BUSY\r\n" + end + smtp = Net::SMTP.start 'localhost', server.port + assert_raise Net::SMTPServerBusy do + smtp.quit! + end + assert_equal "QUIT\r\n", server.commands.last + refute smtp.started? + end + + def test_quit_and_ignore + server = FakeServer.start + def server.quit + @sock.puts "400 BUSY\r\n" + end + smtp = Net::SMTP.start 'localhost', server.port + smtp.quit!(exception: false) + assert_equal "QUIT\r\n", server.commands.last + refute smtp.started? + end + + def test_disconnect + server = FakeServer.start + smtp = Net::SMTP.start 'localhost', server.port + smtp.disconnect + assert_equal "EHLO localhost\r\n", server.commands.last + refute smtp.started? + end + def fake_server_start(**kw) server = FakeServer.new server.start(**kw) From 1db3a9e1e23910348adb18fc076929a0371c786b Mon Sep 17 00:00:00 2001 From: nick evans Date: Wed, 8 May 2024 14:57:27 -0400 Subject: [PATCH 2/2] Add core assertions for `assert_warn` --- .github/workflows/test.yml | 2 +- Gemfile | 1 + test/net/smtp/test_smtp.rb | 16 ++++++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 98f8f4c..31d302e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -31,4 +31,4 @@ jobs: - name: Install dependencies run: bundle install - name: Run test - run: rake test + run: bundle exec rake test diff --git a/Gemfile b/Gemfile index 3dc2883..730174f 100644 --- a/Gemfile +++ b/Gemfile @@ -6,4 +6,5 @@ group :development do gem "bundler" gem "rake" gem "test-unit" + gem "test-unit-ruby-core", git: "https://github.com/ruby/test-unit-ruby-core" end diff --git a/test/net/smtp/test_smtp.rb b/test/net/smtp/test_smtp.rb index 004c303..8abc36a 100644 --- a/test/net/smtp/test_smtp.rb +++ b/test/net/smtp/test_smtp.rb @@ -2,6 +2,9 @@ require 'net/smtp' require 'test/unit' +require "core_assertions" + +Test::Unit::TestCase.include Test::Unit::CoreAssertions module Net class TestSMTP < Test::Unit::TestCase @@ -628,6 +631,19 @@ def test_quit! smtp.quit! end + def test_quit_and_warn + server = FakeServer.start + def server.quit + @sock.puts "400 BUSY\r\n" + end + smtp = Net::SMTP.start 'localhost', server.port + assert_warn(/SMTPServerBusy during .*#quit!/i) do + smtp.quit!(exception: :warn) + end + assert_equal "QUIT\r\n", server.commands.last + refute smtp.started? + end + def test_quit_and_reraise server = FakeServer.start def server.quit