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 broken build #1471

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jkowens
Member

jkowens commented Aug 23, 2018

The latest version of mini_racer does not support ruby 2.2 so that build was failing. There was also a broken Sinatra::Cookie spec that I'm wondering if is related to changes to Sinatra::IndifferentHash.

cc: @mwpastore @namusyaka

@dentarg

This comment has been minimized.

Show comment
Hide comment
@dentarg

dentarg commented Aug 23, 2018

Related to #1455

Replace mini_racer with therubyracer
The mini_racer gem requires Ruby 2.3 or greater so the TravisCI
build for Ruby 2.2 was failing.
@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 23, 2018

Member

Thanks @dentarg. I'm updating this PR to replace mini_racer with therubyracer for now to allow for Ruby 2.2 test builds.

Member

jkowens commented Aug 23, 2018

Thanks @dentarg. I'm updating this PR to replace mini_racer with therubyracer for now to allow for Ruby 2.2 test builds.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Sep 14, 2018

Member

@jkowens Replacement mini_racer with therubyracer looks reasonable, but the changes of cookie spec seems weird.
Could you explain a principle of the failure?

Member

namusyaka commented Sep 14, 2018

@jkowens Replacement mini_racer with therubyracer looks reasonable, but the changes of cookie spec seems weird.
Could you explain a principle of the failure?

@@ -559,7 +559,7 @@ def cookies(*set_cookies)
it 'checks response cookies' do
jar = cookies
jar['foo'] = 'bar'
expect(jar).to include(:foo)

This comment has been minimized.

@jkowens

jkowens Sep 14, 2018

Member

Sinatra::Cookies behaves like a Hash. I'm wondering if there was a change that prevents it from acting like an IndifferentHash? If this is just a regular hash, 'foo' and :foo are different keys.

@jkowens

jkowens Sep 14, 2018

Member

Sinatra::Cookies behaves like a Hash. I'm wondering if there was a change that prevents it from acting like an IndifferentHash? If this is just a regular hash, 'foo' and :foo are different keys.

This comment has been minimized.

@jkowens
@jkowens

This comment has been minimized.

@namusyaka

namusyaka Sep 15, 2018

Member

I'd say that this spec ensure that the cookie should behave like a indifferent hash, right?
Your change doesn't look like essential solve.

@namusyaka

namusyaka Sep 15, 2018

Member

I'd say that this spec ensure that the cookie should behave like a indifferent hash, right?
Your change doesn't look like essential solve.

This comment has been minimized.

@jkowens

jkowens Sep 15, 2018

Member

Right, it does look like that was what it was trying to ensure. However, that is not made clear by the spec description. Technically, it should only be ensuring that include checks the response cookie for 'foo'. If Sinatra::Cookies should behave like IndifferentHash I think a spec that makes that desired behavior explicitly clear should be added. Along with that spec, an update that makes it pass would be good...if that's really what is wanted.

@jkowens

jkowens Sep 15, 2018

Member

Right, it does look like that was what it was trying to ensure. However, that is not made clear by the spec description. Technically, it should only be ensuring that include checks the response cookie for 'foo'. If Sinatra::Cookies should behave like IndifferentHash I think a spec that makes that desired behavior explicitly clear should be added. Along with that spec, an update that makes it pass would be good...if that's really what is wanted.

This comment has been minimized.

@jkowens

jkowens Sep 15, 2018

Member

The built-in include matcher calls to_hash on Sinatra::Cookies which returns a regular Hash. So to test this it would have to either be:

expect(jar.include?(:foo)).to be true

or

expect(jar).to include('foo')

@jkowens

jkowens Sep 15, 2018

Member

The built-in include matcher calls to_hash on Sinatra::Cookies which returns a regular Hash. So to test this it would have to either be:

expect(jar.include?(:foo)).to be true

or

expect(jar).to include('foo')

This comment has been minimized.

@namusyaka

namusyaka Sep 15, 2018

Member

You needed to investigate the problem more deeply.

Please take a look at the rspec issue.
The backword-compatibility breaking isn't intentional, and the original issue was closed by this patch.
However, our problem hasn't been fixed by that because Sinatra::Cookies::Jar doesn't inherit Hash.

The patch seems not to be enough for fixing the breaking.
I think this is the rspec issue, so once I'll try to fix our failure spec by changing our spec.

@namusyaka

namusyaka Sep 15, 2018

Member

You needed to investigate the problem more deeply.

Please take a look at the rspec issue.
The backword-compatibility breaking isn't intentional, and the original issue was closed by this patch.
However, our problem hasn't been fixed by that because Sinatra::Cookies::Jar doesn't inherit Hash.

The patch seems not to be enough for fixing the breaking.
I think this is the rspec issue, so once I'll try to fix our failure spec by changing our spec.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Sep 15, 2018

Member

@jkowens If you're going to send a PR for the spec improvement, please open new PR instead of this.

Member

namusyaka commented Sep 15, 2018

@jkowens If you're going to send a PR for the spec improvement, please open new PR instead of this.

@namusyaka namusyaka closed this Sep 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment