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

add spec for Fiber#raise #809

Merged
merged 2 commits into from
Nov 5, 2020
Merged

add spec for Fiber#raise #809

merged 2 commits into from
Nov 5, 2020

Conversation

kuei0221
Copy link
Contributor

This PR is for solving #745, add spec of Fiber#raise. I am not very familiar with Fiber, so tell me if anything needs to improve 😄

Added Fiber#raise that behaves like Fiber#resume but raises an
exception on the resumed fiber. Feature #10344

@@ -0,0 +1,91 @@
require_relative '../../spec_helper'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fiber#raise is actually a core method (it does not need require 'fiber' unlike e.g. Fiber#transfer), so it should be in core/fiber/raise_spec.rb

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a good start but needs some fixes.

-> { fiber.raise }.should raise_error(FiberError, "dead fiber called")
end

class TestError < StandardError ; end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will define a new global constant, it should instead be nested under module FiberSpecs, or Class.new(StandardError) be used instead.

ruby_version_is "2.7" do
describe "Fiber#raise" do

it 'raise RuntimeError by default' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec descriptions should form a sentence, so typically the first word will be a 3rd person present verb like raises/accepts/does not accept, etc.

@eregon
Copy link
Member

eregon commented Oct 25, 2020

There are specs for Kernel/Thread#raise (https://github.com/ruby/spec/blob/master/shared/kernel/raise.rb) we should be able to reuse here, like so:

module FiberSpecs
  class NewFiberToRaise
    def self.raise(*args)
      fiber = Fiber.new { Fiber.yield }
      fiber.resume
      fiber.raise(*args)
    end
  end
end

  describe "Fiber#raise" do
    it_behaves_like :kernel_raise, :raise, FiberSpecs::NewFiberToRaise
  end

@kuei0221
Copy link
Contributor Author

kuei0221 commented Nov 4, 2020

Thanks for your comment! I have followed your suggestion and update the code, everything works fine except one.
One of the shared test will fail:

Fiber#raise re-raises a previously rescued exception without overwriting the backtrace FAILED
Expected "/Users/michaelhuang/spec/core/fiber/fixtures/classes.rb:4:in `yield'"
to include "/Users/michaelhuang/spec/shared/kernel/raise.rb:55:"
/Users/michaelhuang/spec/shared/kernel/raise.rb:66:in `rescue in rescue in block (2 levels) in <top (required)>'
/Users/michaelhuang/spec/shared/kernel/raise.rb:57:in `rescue in block (2 levels) in <top (required)>'
/Users/michaelhuang/spec/shared/kernel/raise.rb:54:in `block (2 levels) in <top (required)>'
/Users/michaelhuang/spec/core/fiber/raise.rb:6:in `block in <top (required)>'
/Users/michaelhuang/spec/core/fiber/raise.rb:5:in `<top (required)>'

I think Fiber has a different mechanism here.
In the case of Kernel or Thread (other class also shared :kernel_raise), an exception will happen at the file where raise was called.

Fiber does not, it will jump to the file where Fiber yield was and throw an exception. Not active by raise.
If my understanding is correct, I will have to skip this test when class is Fiber, or maybe isolate it to another shared example. And then add another test to cover this feature.

@eregon
Copy link
Member

eregon commented Nov 4, 2020

@kuei0221 I think that's simply of the extra indirection we have for Fiber#raise with the FiberSpecs::NewFiberToRaise.raise helper.
Could you push what you have so far?
Then I'll find a way to address that spec.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the new specs!

@eregon eregon merged commit 11cb089 into ruby:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants