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.

Sign up for GitHub

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

Jump to bottom

Fix broken build #1471

Closed
wants to merge 2 commits into from
Closed

Fix broken build #1471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
397ab01
Fix broken cookie spec
jkowens Aug 23, 2018
ee36d53
Replace mini_racer with therubyracer
jkowens Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ end

if RUBY_ENGINE == "ruby"
gem 'less', '~> 2.0'
gem 'mini_racer'
gem 'therubyracer'
gem 'redcarpet'
gem 'wlang', '>= 2.0.1'
gem 'bluecloth'
Expand Down
2 changes: 1 addition & 1 deletion sinatra-contrib/spec/cookies_spec.rb
View file
Open in desktop
Expand Up @@ -559,7 +559,7 @@ def cookies(*set_cookies)
it 'checks response cookies' do
jar = cookies
jar['foo'] = 'bar'
expect(jar).to include(:foo)
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@namusyaka ^

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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')

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

it 'does not use deleted cookies' do
Expand Down