-
-
Notifications
You must be signed in to change notification settings - Fork 530
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
Introduce suspenders:testing
generator
#1156
Introduce suspenders:testing
generator
#1156
Conversation
def run_rspec_installation_script | ||
rails_command "generate rspec:install" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure it's best to lean on what RSpec gives us in an effort to avoid drift. We'll just modify as little as possible.
@seanpdoyle tagging you in case you think there's anything else worth adding to |
I'd rather see a |
I like the idea of offering the ability to chose between test suites, since not all projects will have the same requirements. It's possible a client might have more familiarity with one suite over the other. That being said, this could be a great opportunity to challenge our existing preferences by defaulting to Minitest over RSpec for new Rails applications. My vision for Suspenders Additionally, RSpec needs to play catch-up with Rails as new features are added. For example, Rails ships with testing helpers for Action Cable. When that was introduced, RSpec needed to add it, and is also burdened to keep parity with Rails. |
Worth noting rspec still doesn't support parallel testing by default. |
I have a strong preference for RSpec. I think it has more helpful test output and a nice variety of matchers. I'm not too concerned about parallel testing as my choice of CI has parallel testing support anyway. |
This would be the only Suspenders generator that offers a choice, which deviates from the goal of having Suspenders be the codification of our technical decisions. TBH, I'm not sure what I'd use Suspenders for if it isn't making the choice for me. |
#1145 also introduces a choice as well. Maybe we need to clean that up too 🤷 EDIT: Although, it provides a default. |
I agree that suspenders should be opinionated and choose one, and I (shockingly to myself of two years ago) think it should prefer Minitest. After using it on Closeknit and finding it no harder to work with than rspec (ergonomics was my main concern) I find myself enamored with ruby's built-in testing framework, no extra metaphors, just ruby code. Also feels easier to back out of if one truly wants rspec. |
I don't have nearly as much experience using Minitest, but I'm in the camp of having it as our default—eventually. I'm a fan of one less dependency and a test framework that doesn't need to play catch up, amongst other pros. Eventually because most of the team is far more comfortable with RSpec. The needs here may be small, but I'm curious of the other side of this conversation: what we may need to do to smooth a transition to Minitest. Part of having suspenders—to @mike-burns's point—is to remove need to make decisions, but also for consistency across projects. Would having both options further the divide of projects using different testing tools? |
Minitest is great, but I don't think we should use it by default. The reason is that it's too barebones and requires helper gems to come a bit closer to what RSpec provides by default, such as advanced matchers, run only failed tests, full-fledged test runner with documentation, etc. Every time I use minitest on a reasonable project, I add more and more gems to cover my needs. I actually love Minitest, but it has these gotchas! Also, thoughtbot is already familiar with RSpec. There is a lot of learning material in Upcase, our blog, etc. |
I'd like to think that MiniTest needs to play catch up with RSpec 😄 . Stubbing, mocking, extremely convenient matchers, etc. Generally, i think there has to be a really compelling reason to switch testing tools. If it helps create better tests with less common gotchas (less mystery guests, instance variables, mixins) I would be game, but if its just that its part of Rails stack or that its in ruby, that's not enough. |
+1 on a compelling reason. Writing more maintainable tests has historically been an argument in favor of Minitest. May have changed over the years, but I also remember preferring/missing mocking in RSpec. @thiagoa what gems do you find yourself adding on every project? They could be a part of the defaults. I must have my |
In my opinion, this is the most compelling reason. #1156 (comment) shares a great example. To share a concrete example, 3 years ago, Rails recently introduced the concept of an Error Reporting object as part of the 7.0 release. Alongside that concept, it also added some built-in assertions as part of ActiveSupport::Testing::ErrorReporterAssertions. Does RSpec Rails have a similar set of matchers? Out of the box, Rails provides built-in general purpose Minitest extensions as part of ActiveSupport::Testing::Assertions, and other utilities like ActiveSupport::Testing::TimeHelpers and ActiveSupport::LogSubscriber::TestHelper. Along with those universal utilities, there are framework specific test cases, like:
Along with those test cases, there are framework specific helpers to mix-and-match, like:
Since these are all part of the framework, they don't require additional layers of abstraction or other integration touchpoint. Most In my mind, consuming the helpers directly from Rails means teams are more encouraged to read the Rails Guides and API documentation. If we find the tooling to be poorly documented, or behaving in unexpected ways, making those improvements upstream will benefit a larger group of end-users than making improvements to a project like RSpec that's a downstream dependency. (I also strongly encourage everyone to make documentation and implementation improvements to RSpec too!). Learning and using Minitest had the largest impact on my abilities to make contributions to |
@emilford I don't remember, as I don't regularly work with Minitest projects. But at the very minimum, a better test runner and better matchers for more powerful and expressive assertions (maybe that changed?). I'm pretty sure error reporting is better on RSpec as well. I mostly used Minitest in small-scale personal projects, otherwise, I always pulled additional gems. |
To echo what others are saying, if we think Minitest is better but it requires more setup and gems installed, that's exactly what we should be doing in Suspenders. |
8b00d2d
to
2b7c5c7
Compare
After reviewing feedback in this pull request, I decided to keep RSpec over the default Rails test framework for the following reasons:
However, since Suspenders is a living, breathing representation of thoughtbot's opinions, we can continue to explore if and how we would want to use the default Rails test framework by making necessary modifications to meet our |
allow: [ | ||
/(chromedriver|storage).googleapis.com/, | ||
"googlechromelabs.github.io", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found I needed to add googlechromelabs.github.io
and adjust the original host when I tested this on a fresh Rails app. Otherwise, we would raise WebMock::NetConnectNotAllowedError
.
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test`, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. [headless]:drive://github.com/rails/rails/pull/50512 [rspec]: rspec/rspec-rails#2746
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test`, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. Ensure all files under `spec/support` are loaded by uncommenting a line generated by the RSpec installation script. [headless]:drive://github.com/rails/rails/pull/50512 [rspec]: rspec/rspec-rails#2746
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test`, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. Ensure all files under `spec/support` are loaded by uncommenting a line generated by the RSpec installation script. [headless]: drive://github.com/rails/rails/pull/50512 [rspec]: rspec/rspec-rails#2746
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test`, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. Ensure all files under `spec/support` are loaded by uncommenting a line generated by the RSpec installation script. [headless]: rails/rails#50512 [rspec]: rspec/rspec-rails#2746
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test` flag, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. Ensure all files under `spec/support` are loaded by uncommenting a line generated by the RSpec installation script. [headless]: rails/rails#50512 [rspec]: rspec/rspec-rails#2746
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test` flag, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. Ensure all files under `spec/support` are loaded by uncommenting a line generated by the RSpec installation script. [headless]: rails/rails#50512 [rspec]: rspec/rspec-rails#2746
Configures test environment. This differs from #1156 in that this commit is concerned with configuration, where that commit was concerned with generating a holistic test suite. It's also possible to run each generator independently, and the two should not rely on one another. Disables [action_dispatch.show_exceptions][] in an effort to [improve failure output][comment]. Enables [raise_on_missing_translations][] to keep parity with the same setting in development #1149 [action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions [comment]: #1149 (comment) [raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
Configures test environment. This differs from #1156 in that this commit is concerned with configuration, where that commit was concerned with generating a holistic test suite. It's also possible to run each generator independently, and the two should not rely on one another. Disables [action_dispatch.show_exceptions][] in an effort to [improve failure output][comment]. Enables [raise_on_missing_translations][] to keep parity with the same setting in development #1149 [action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions [comment]: #1149 (comment) [raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
Configures test environment. This differs from #1156 in that this commit is concerned with configuration, where that commit was concerned with generating a holistic test suite. It's also possible to run each generator independently, and the two should not rely on one another. Disables [action_dispatch.show_exceptions][] in an effort to [improve failure output][comment]. Enables [raise_on_missing_translations][] to keep parity with the same setting in development #1149 [action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions [comment]: #1149 (comment) [raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
Configures test environment. This differs from #1156 in that this commit is concerned with configuration, where that commit was concerned with generating a holistic test suite. It's also possible to run each generator independently, and the two should not rely on one another. Disables [action_dispatch.show_exceptions][] in an effort to [improve failure output][comment]. Enables [raise_on_missing_translations][] to keep parity with the same setting in development #1149 [action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions [comment]: #1149 (comment) [raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
Set up projects for an in-depth test-driven development workflow. Installs and configures [rspec-rails][], [action_dispatch-testing-integration-capybara][], [shoulda-matchers][], [webdrivers][] and [webmock][]. [rspec-rails]: https://github.com/rspec/rspec-rails [action_dispatch-testing-integration-capybara]: https://github.com/thoughtbot/action_dispatch-testing-integration-capybara [shoulda-matchers]: https://github.com/thoughtbot/shoulda-matchers [webdrivers]: https://github.com/titusfortner/webdrivers [webmock]: https://github.com/bblimke/webmock ## Details Generate `spec/rails_helper.rb` and `spec/spec_helper.rb` via `rails g rspec:intall` in an effort to not drift from what RSpec recommends out of the box. ```ruby #spec/spec_helper.rb RSpec.configure do |config| config.example_status_persistence_file_path = "tmp/rspec_examples.txt" config.order = :random config.expect_with :rspec do |expectations| expectations.include_chain_clauses_in_custom_matcher_descriptions = true end config.mock_with :rspec do |mocks| mocks.verify_partial_doubles = true end config.shared_context_metadata_behavior = :apply_to_host_groups end WebMock.disable_net_connect!( allow_localhost: true, allow: [ /(chromedriver|storage).googleapis.com/, "googlechromelabs.github.io", ] ) ``` The only thing that differs from the existing `spec/rails_helper.rb` configuration is: ```ruby config.infer_base_class_for_anonymous_controllers = false ``` ```ruby # spec/support/chromedriver.rb require "selenium/webdriver" Capybara.register_driver :chrome do |app| Capybara::Selenium::Driver.new(app, browser: :chrome) end Capybara.register_driver :headless_chrome do |app| options = ::Selenium::WebDriver::Chrome::Options.new options.headless! options.add_argument "--window-size=1680,1050" Capybara::Selenium::Driver.new app, browser: :chrome, options: options end Capybara.javascript_driver = :headless_chrome RSpec.configure do |config| config.before(:each, type: :system) do driven_by :rack_test end config.before(:each, type: :system, js: true) do driven_by Capybara.javascript_driver end end ``` ```ruby # spec/support/shoulda_matchers.rb Shoulda::Matchers.configure do |config| config.integrate do |with| with.test_framework :rspec with.library :rails end end ``` ```ruby # spec/support/i18n.rb RSpec.configure do |config| config.include ActionView::Helpers::TranslationHelper end ``` ```ruby # spec/support/action_mailer.rb RSpec.configure do |config| config.before(:each) do ActionMailer::Base.deliveries.clear end end ``` ## Notable changes This commit removes the [formulaic][] dependency. A follow-up commit could explore creating a separate one-off generator for this, but for now, we're aiming for the leanest build possible. [formulaic]: https://github.com/calebhearth/formulaic
Follow-up to #1156 Creates parity with Rails' decision to [use a headless driver by default][headless]. This will be fixed in an [upcoming release][rspec] of rspec-rails, but I felt it was important to capture here. Additionally, it ensures the `screen_size` is the same as what is set in Rails. Removes `webdrivers` dependency in favor of `selenium-webdriver`. This generator assumes the app was generated with the `--skip-test` flag, which means we need to add the `selenium-webdriver` and `capybara` gems. Updates `action_dispatch-testing-integration-capybara` dependency to the most recent tagged release in an effort to suppress Dependabot notifications. Ensure all files under `spec/support` are loaded by uncommenting a line generated by the RSpec installation script. [headless]: rails/rails#50512 [rspec]: rspec/rspec-rails#2746
Configures test environment. This differs from #1156 in that this commit is concerned with configuration, where that commit was concerned with generating a holistic test suite. It's also possible to run each generator independently, and the two should not rely on one another. Disables [action_dispatch.show_exceptions][] in an effort to [improve failure output][comment]. Enables [raise_on_missing_translations][] to keep parity with the same setting in development #1149 [action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions [comment]: #1149 (comment) [raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
Set up projects for an in-depth test-driven development workflow.
Installs and configures rspec-rails,
action_dispatch-testing-integration-capybara, shoulda-matchers,
webdrivers and webmock.
After reviewing feedback in this pull request, I decided to keep RSpec over the
default Rails test framework for the following reasons:
familiar with Minitest.
However, since Suspenders is a living, breathing representation of thoughtbot's
opinions, we can continue to explore if and how we would want to use the
default Rails test framework by making necessary modifications to meet our
needs.
Details
Generate
spec/rails_helper.rb
andspec/spec_helper.rb
viarails g rspec:intall
in an effort to not drift from what RSpec recommends out of the box.
The only thing that differs from the existing
spec/rails_helper.rb
configuration is:Notable changes
This commit removes the formulaic dependency. A
follow-up commit could explore creating a separate one-off generator for
this, but for now, we're aiming for the leanest build possible.