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

Test with rack-test 2.0 #2279

Closed
wants to merge 1 commit into from
Closed

Conversation

terceiro
Copy link

Those two tests endpoint_spec are in the end testing internal details of
rack-test. Change them so that they still test what was intended, but in
a more robust way that does not break on changes on the implementation
details of rack-test.

Fixes: #2278

Those two tests endpoint_spec are in the end testing internal details of
rack-test. Change them so that they still test what was intended, but in
a more robust way that does not break on changes on the implementation
details of rack-test.

Fixes: ruby-grape#2278
@grape-bot
Copy link

1 Warning
⚠️ Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#2279](https://github.com/ruby-grape/grape/pull/2279): Test with rack-test 2.0 - [@terceiro](https://github.com/terceiro).

Generated by 🚫 Danger

@@ -432,7 +431,7 @@ def app
end
post '/upload', { file: '' }, 'CONTENT_TYPE' => 'multipart/form-data; boundary=foobar'
expect(last_response.status).to eq(400)
expect(last_response.body).to eq('empty message body supplied with multipart/form-data; boundary=foobar content-type')
expect(last_response.body).not_to be_empty
Copy link
Member

Choose a reason for hiding this comment

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

Are we losing something in this test? Is there something we can assert that ensures that the error is empty message body?

Copy link
Author

Choose a reason for hiding this comment

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

good question. TBH I could not figure out yet where this message in the body comes from. I thought it came from rack-test, but it doesn't. It seems that the new rack-test changes the way it composes requests with file uploads though.

@dblock
Copy link
Member

dblock commented Sep 27, 2022

Want to finish this @terceiro ?

@ericproulx ericproulx mentioned this pull request Jan 4, 2023
@dblock dblock closed this in #2302 Jan 5, 2023
@terceiro
Copy link
Author

terceiro commented Jan 5, 2023

was this closed by mistake? I think we should get support for rack-test 2.0 at some point

@terceiro
Copy link
Author

terceiro commented Jan 5, 2023

no, sorry for the noise. that MR does seem to address the issues here

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.

test failure with rack-test 2.0
3 participants