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

Fix: TypeError: can't convert ImageOptim::Timer into Float #194

Merged
merged 2 commits into from
Oct 3, 2021
Merged

Fix: TypeError: can't convert ImageOptim::Timer into Float #194

merged 2 commits into from
Oct 3, 2021

Conversation

yahonda
Copy link
Contributor

@yahonda yahonda commented Oct 2, 2021

This pull request addresses TypeError: can't convert ImageOptim::Timer into Float
when the timeout is an instance of ImageOptim::Timer with Ruby 3.0.2.

Refer to the original issue at Discourse https://meta.discourse.org/t/optimizedimage-specs-are-failing-with-ruby-3-0-2/204930

Current behavior

  • Reverting this code from this pull request to see how it fails.
$ git diff
diff --git a/lib/image_optim/cmd.rb b/lib/image_optim/cmd.rb
index 4957f8d..ea00a28 100644
--- a/lib/image_optim/cmd.rb
+++ b/lib/image_optim/cmd.rb
@@ -17,7 +17,7 @@ class ImageOptim
           pid = Process.spawn(*args)

           waiter = Process.detach(pid)
-          if waiter.join(timeout.to_f)
+          if waiter.join(timeout)
             status = waiter.value

             check_status!(status)
$
  • Ruby 3.0.2 raises TypeError
$ ruby -v
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
$ bundle exec rspec ./spec/image_optim/cmd_spec.rb:52
Run options: include {:locations=>{"./spec/image_optim/cmd_spec.rb"=>[52]}}

Randomized with seed 32230
F

Failures:

  1) ImageOptim::Cmd.run with timeout returns process success status when timeout is instance of ImageOptim::Timer
     Failure/Error: if waiter.join(timeout)

     TypeError:
       can't convert ImageOptim::Timer into Float
     # ./lib/image_optim/cmd.rb:20:in `join'
     # ./lib/image_optim/cmd.rb:20:in `run'
     # ./spec/image_optim/cmd_spec.rb:54:in `block (4 levels) in <top (required)>'

Finished in 0.0023 seconds (files took 0.09478 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/image_optim/cmd_spec.rb:52 # ImageOptim::Cmd.run with timeout returns process success status when timeout is instance of ImageOptim::Timer

Randomized with seed 32230

$
  • Ruby 2.7.4 works fine
$ ruby -v
ruby 2.7.4p191 (2021-07-07 revision a21a3b7d23) [x86_64-linux]
yahonda@myryzen:~/src/github.com/toy/image_optim$ bundle exec rspec ./spec/image_optim/cmd_spec.rb:52
Run options: include {:locations=>{"./spec/image_optim/cmd_spec.rb"=>[52]}}

Randomized with seed 21473
.

Finished in 0.00287 seconds (files took 0.09183 seconds to load)
1 example, 0 failures

Randomized with seed 21473

$

@toy
Copy link
Owner

toy commented Oct 2, 2021

Can confirm on 3.0.0, 3.0.1 and 3.0.2. Something got broken in the rb_to_float C method for converting objects to float, from documentation having #to_f should be enough (1, 2), but following is broken in all 3.0 versions:

class KindOfFloat
  def initialize(value)
    @value = value.to_f
  end

  def to_f
    @value
  end
end

Thread.new{ }.join(KindOfFloat.new(0.1))

@toy
Copy link
Owner

toy commented Oct 2, 2021

Thank you for contribution! I will submit an issue to ruby and check this PR

@toy
Copy link
Owner

toy commented Oct 2, 2021

I've temporarily fixed the issue caused by rubocop (false positive fixed in rubocop/rubocop#10141) and created an issue in ruby bug tracker.

PR looks good, please add an entry to changelog and rebase.

@yahonda
Copy link
Contributor Author

yahonda commented Oct 3, 2021

Thanks for the review. Added a changelog entry and rebased and squashed from the latest master branch.

@toy toy merged commit 95f19f9 into toy:master Oct 3, 2021
@toy
Copy link
Owner

toy commented Oct 3, 2021

Thanks again, released v0.31.0

@yahonda yahonda deleted the ruby3 branch October 4, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants