Skip to content

Remove dev dependency to Thin #479

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

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Conversation

davidstosik
Copy link
Contributor

@davidstosik davidstosik commented Oct 2, 2020

This change is part of preliminary efforts at attempting to refresh
the Continuous Integration setup, by supporting recent Ruby versions and
updating dependencies.

To be honest, I don't know what benefits the Thin server brings to the table.
I tried digging in the code, but the commit that introduced it does not
tell me much: cdeea8b.

What I know however is that I have troubles running the specs locally,
getting a timeout on this line:

Rack::Handler::Thin.run(app, :Port => port)

However, letting Capybara handle its server details, as shown in this PR,
does not seem to have any negative impact (spec is still green, and fairly fast).

This change is part of preliminary efforts at attempting to refresh
the Continuous Integration setup, by supporting recent Ruby version and
updating dependencies.

To be honest, I don't know what benefits the Thin server brings to the table.
I tried digging in the code, but the commit that introduced it does not
tell me much: cdeea8b.

What I know however is that I have troubles running the specs locally,
getting a timeout on this line:

https://github.com/zipmark/rspec_api_documentation/blob/560c3bdc7bd5581e7c223334390221ecfc910be8/spec/http_test_client_spec.rb#L16

However, letting Capybara handle its server details, as shown in this PR,
does not seem to have any negative impact (spec is still green, and fairly fast).
@jakehow
Copy link
Member

jakehow commented Oct 2, 2020

@davidstosik I don't recall, but I'm guessing it was to avoid running webrick back in 2014. From Capybara's source at the time:

 # Register a proc that Capybara will call to run the Rack application.
    #
    #     Capybara.server do |app, port|
    #       require 'rack/handler/mongrel'
    #       Rack::Handler::Mongrel.run(app, :Port => port)
    #     end
    #
    # By default, Capybara will try to run webrick.
    #
    # @yield [app, port]                      This block receives a rack app and port and should run a Rack handler

From: https://github.com/teamcapybara/capybara/blob/3285e11fb0c6bee2773358bcfa35fc899de4b2a6/lib/capybara.rb

Looks like capybara now uses puma by default which seems fine to me.

@davidstosik
Copy link
Contributor Author

davidstosik commented Oct 2, 2020

Thanks for linking to that! This dates from a time I had barely discovered Ruby and Rails, so I was not aware. 🙇‍♂️

Looks like capybara now uses puma by default which seems fine to me.

This would require adding the puma gem to the dependencies. Until we can get the test suite to run in current Ruby versions (2.5, 2.6, 2.7), I'd rather avoid adding additional dev dependencies (risking that latest versions might not work with the Ruby versions currently run on Travis, and also that using oldest versions would make the upgrade path more difficult).

In the Capybara version used so far (locked in the gemspec), Webrick is used. I'm currently trying to update more (PR to come), and before opting for Puma, I'm thinking of keeping Webrick with Capybara.server = :webrick.

avoid running webrick back in 2014

Curious about what could have been the reason... 🤔
Was webrick bad?

@jakehow
Copy link
Member

jakehow commented Oct 2, 2020

Webrick had a bad reputation, but I think that was mostly earned because it did not work in production. Pre-mongrel there was not great ways to run rails apps. Thin/Puma/Unicorn all descended from mongrel. For our purposes it seems perfectly reasonable. It is also pure ruby so for something like CI it should work as long as the right version is pinned across rubies.

@jakehow jakehow merged commit 2e469c4 into zipmark:master Oct 2, 2020
@davidstosik davidstosik deleted the remove-thin branch October 2, 2020 06:56
E1337Kat pushed a commit to E1337Kat/rspec_api_documentation that referenced this pull request Mar 5, 2024
This change is part of preliminary efforts at attempting to refresh
the Continuous Integration setup, by supporting recent Ruby version and
updating dependencies.

To be honest, I don't know what benefits the Thin server brings to the table.
I tried digging in the code, but the commit that introduced it does not
tell me much: cdeea8b.

What I know however is that I have troubles running the specs locally,
getting a timeout on this line:

https://github.com/zipmark/rspec_api_documentation/blob/560c3bdc7bd5581e7c223334390221ecfc910be8/spec/http_test_client_spec.rb#L16

However, letting Capybara handle its server details, as shown in this PR,
does not seem to have any negative impact (spec is still green, and fairly fast).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants