Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Puma::NullIO consistency with real IO #3276

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

casperisfine
Copy link
Contributor

This isn't a big deal, but every small difference has a chance to let a bug slip as it's likely not every scenario is tested with both NullIO and a real IO.

@MSP-Greg
Copy link
Member

Thanks for the PR. Five of the tests fail when only the test changes are added to master.

LGTM. Any thoughts on the below, which removes two asserts inside of assert_raises blocks, and changes ::Tempfile.new to ::Tempfile.create? Don't feel strongly about them...

diff --git a/test/test_null_io.rb b/test/test_null_io.rb
index a3b2a2b5..dd86ffc8 100644
--- a/test/test_null_io.rb
+++ b/test/test_null_io.rb
@@ -77,11 +77,11 @@ class TestNullIO < Minitest::Test
     assert_includes error.message, "no implicit conversion of Object into String"
 
     error = assert_raises TypeError do
-      assert_equal "", nio.read(0, Object.new)
+      nio.read(0, Object.new)
     end
 
     error = assert_raises TypeError do
-      assert_nil nio.read(1, Object.new)
+      nio.read(1, Object.new)
     end
     assert_includes error.message, "no implicit conversion of Object into String"
   end
@@ -133,7 +133,7 @@ end
 # ensure all the test behavior is accurate
 class TestNullIOConformance < TestNullIO
   def setup
-    self.nio = ::Tempfile.new
+    self.nio = ::Tempfile.create
     nio.sync = true
   end
 
@@ -141,4 +141,10 @@ class TestNullIOConformance < TestNullIO
     self.nio = StringIO.new
     super
   end
+
+  def teardown
+    return unless nio.is_a? ::File
+    nio.close
+    File.unlink nio.path
+  end
 end

@casperisfine
Copy link
Contributor Author

removes two asserts inside of assert_raises blocks

Yup oversight on my part. Thanks for catching it.

This isn't a big deal, but every small difference has a chance to
let a bug slip as it's likely not every scenario is tested with both
NullIO and a real IO.
@casperisfine
Copy link
Contributor Author

PR updated.

@MSP-Greg MSP-Greg added the bug label Nov 16, 2023
@MSP-Greg
Copy link
Member

@casperisfine Thanks again for the PR. As you stated "but every small difference has a chance to let a bug slip"

@MSP-Greg MSP-Greg merged commit 1eb59ba into puma:master Nov 16, 2023
55 of 64 checks passed
@MSP-Greg
Copy link
Member

@casperisfine

Oversights. I always work locally with Ruby head builds, as I feel I should 'walk what I talk'.

A few tests check the response for a frozen IO, which should raise FrozenError. But FrozenError is not defined in Ruby 2.4.

My mistake. Note to self - always review the CI.

<Rant on> why are we supporting four EOL Ruby versions? <Rant off>

I'll fix...

@casperisfine
Copy link
Contributor Author

Ah my bad, I should have paid attention to CI too.

FrozenError is not defined in Ruby 2.4.

Oh wow, crazy, I didn't remember FrozenError being such a novelty, but indeed:

Capture d’écran 2023-11-16 à 16 55 21

@MSP-Greg
Copy link
Member

MSP-Greg commented Nov 16, 2023

I'm working on a fix, some non-MRI errors also. I'm not going to start on the EOL Ruby thing.

Oh wow, crazy, I didn't remember FrozenError being such a novelty, but indeed

I don't think it's fair to expect anyone to remember that. I certainly didn't.

nateberkopec pushed a commit that referenced this pull request Jan 2, 2024
This isn't a big deal, but every small difference has a chance to
let a bug slip as it's likely not every scenario is tested with both
NullIO and a real IO.

Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
nateberkopec pushed a commit that referenced this pull request Jan 2, 2024
This isn't a big deal, but every small difference has a chance to
let a bug slip as it's likely not every scenario is tested with both
NullIO and a real IO.

Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
nateberkopec pushed a commit that referenced this pull request Jan 2, 2024
This isn't a big deal, but every small difference has a chance to
let a bug slip as it's likely not every scenario is tested with both
NullIO and a real IO.

Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants