From c7a7d97648ee268e7996e6a6790f78fcb14467c3 Mon Sep 17 00:00:00 2001 From: atosbucket Date: Tue, 2 Aug 2022 14:24:27 -0400 Subject: [PATCH] Improve upon test suite flakiness (#327) * Make `turbo_test.rb` with Rails' generated `test_helper.rb` > Something in the test suite configuration is preventing the database > from being wiped between test runs. This results in state leaking > between tests. As a result, our Continuous Integration tests are flaky. > > - [hotwired/turbo-rails#248][] As a follow-up to the [short-term solution][] shipped in [hotwired/turbo-rails#248][], this commit attempts to make the `test/turbo_test.rb` file's setup consistent with the test harness setup generated by Rails' [engine generator][] code. To that end, this commit: * renames the `test/turbo_test.rb` file to `test/test_helper.rb` * omits one-off `require` calls for particular dependencies * re-orders the require calls so that the `../test/dummy/config/environment` file is required ahead of the `rails/test_help` file [engine generator]: https://github.com/rails/rails/blob/3c48b4030adbded21bebaa0d78254216cca48a6e/railties/lib/rails/generators/rails/plugin/templates/test/test_helper.rb.tt [hotwired/turbo-rails#248]: https://github.com/hotwired/turbo-rails/pull/248 [short-term solution]: https://github.com/hotwired/turbo-rails/commit/c2dc5b15acaf3cb0aebb581b7dfa0b792ec50bc1 * Use Ruby 2.7 argument forwarding Switching to argument forwarding addresses deprecation warnings like: ``` hotwired/turbo-rails/test/streams/streams_channel_test.rb:140: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call hotwired/turbo-rails/test/turbo_test.rb:14: warning: The called method `render' is defined here ``` * Tests: Load 6.1 defaults in Dummy Application Resolve deprecation warnings like: ``` Preparing test database DEPRECATION WARNING: Non-URL-safe CSRF tokens are deprecated. Use 6.1 defaults or above. (called from at /home/runner/work/turbo-rails/turbo-rails/test/dummy/config/initializers/inspect_helpers.rb:1) DEPRECATION WARNING: Using legacy connection handling is deprecated. Please set `legacy_connection_handling` to `false` in your application. The new connection handling does not support `connection_handlers` getter and setter. Read more about how to migrate at: https://guides.rubyonrails.org/active_record_multiple_databases.html#migrate-to-the-new-connection-handling (called from at /home/runner/work/turbo-rails/turbo-rails/test/test_helper.rb:6) ``` Since our GitHub CI matrix includes `6.1`, `7.0`, and `main`, CI's tests should run with at least the `6.1` defaults. * CI: Continue other executions on error Remove the `continue-on-error` configuration and instead allow other jobs complete in spite of failures. * Improve Flaky Test: Clear fields before filling in Resolve a flaky System Test by ensuring that an input is clear before filling in a new value. * Improve flaky tests: Broadcasts First, don't render HTML with the `` element. Instead, append the element when clicking a ` diff --git a/test/dummy/app/views/messages/echo.html.erb b/test/dummy/app/views/messages/echo.html.erb index ceace5e..b73139a 100644 --- a/test/dummy/app/views/messages/echo.html.erb +++ b/test/dummy/app/views/messages/echo.html.erb @@ -1,5 +1,8 @@

Echo Messages

-<%= turbo_stream_from "messages", channel: EchoChannel, data: {message: "Hello, world!"} %> +<%= render "template_button", content: "Start listening for broadcasts" do %> + <%= turbo_stream_from "messages", channel: EchoChannel, data: {message: "Hello, world!"} %> +<% end %> +
diff --git a/test/dummy/app/views/messages/index.html.erb b/test/dummy/app/views/messages/index.html.erb index 10ee7e9..4eb53da 100644 --- a/test/dummy/app/views/messages/index.html.erb +++ b/test/dummy/app/views/messages/index.html.erb @@ -1,9 +1,8 @@

Messages

- - <%= @messages.count %> messages sent - +<%= render "template_button", content: "Start listening for broadcasts" do %> + <%= turbo_stream_from "messages" %> +<% end %> -<%= turbo_stream_from "messages" %>
diff --git a/test/dummy/app/views/users/profiles/index.html.erb b/test/dummy/app/views/users/profiles/index.html.erb index a3c19a5..e5fa355 100644 --- a/test/dummy/app/views/users/profiles/index.html.erb +++ b/test/dummy/app/views/users/profiles/index.html.erb @@ -1,4 +1,8 @@

Users::Profiles

-<%= turbo_stream_from "users_profiles" %> -
+<%= render "template_button", content: "Start listening for broadcasts" do %> + <%= turbo_stream_from "users_profiles" %> +<% end %> + +
+
diff --git a/test/dummy/config/application.rb b/test/dummy/config/application.rb index fefb235..9d1ed2f 100644 --- a/test/dummy/config/application.rb +++ b/test/dummy/config/application.rb @@ -12,7 +12,7 @@ module Dummy class Application < Rails::Application # Initialize configuration defaults for originally generated Rails version. - config.load_defaults 6.0 + config.load_defaults 6.1 # Settings in config/environments/* take precedence over those specified here. # Application configuration can go into files in config/initializers diff --git a/test/dummy/config/cable.yml b/test/dummy/config/cable.yml index 1cd0f83..663e871 100644 --- a/test/dummy/config/cable.yml +++ b/test/dummy/config/cable.yml @@ -2,7 +2,7 @@ development: adapter: async test: - adapter: async + adapter: inline production: adapter: redis diff --git a/test/frames/frame_request_controller_test.rb b/test/frames/frame_request_controller_test.rb index e43cd60..3ba6fa8 100644 --- a/test/frames/frame_request_controller_test.rb +++ b/test/frames/frame_request_controller_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" class Turbo::FrameRequestControllerTest < ActionDispatch::IntegrationTest test "frame requests are rendered without a layout" do diff --git a/test/frames/frames_helper_test.rb b/test/frames/frames_helper_test.rb index 473b42b..678b540 100644 --- a/test/frames/frames_helper_test.rb +++ b/test/frames/frames_helper_test.rb @@ -1,8 +1,6 @@ -require "turbo_test" +require "test_helper" class Turbo::FramesHelperTest < ActionView::TestCase - setup { Message.delete_all } - test "frame with src" do assert_dom_equal %(), turbo_frame_tag("tray", src: "/trays/1") end diff --git a/test/initializers/helpers_test.rb b/test/initializers/helpers_test.rb index 4f3c56a..ce5ac03 100644 --- a/test/initializers/helpers_test.rb +++ b/test/initializers/helpers_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" class Turbo::HelpersInInitializersTest < ActionDispatch::IntegrationTest test "AC::Base has the helpers in place when initializers run" do diff --git a/test/native/navigation_controller_test.rb b/test/native/navigation_controller_test.rb index da7a2f4..92e1b01 100644 --- a/test/native/navigation_controller_test.rb +++ b/test/native/navigation_controller_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" class Turbo::Native::NavigationControllerTest < ActionDispatch::IntegrationTest test "recede, resume, or refresh when native or redirect when not" do diff --git a/test/streams/broadcastable_test.rb b/test/streams/broadcastable_test.rb index daf1e29..7dd757c 100644 --- a/test/streams/broadcastable_test.rb +++ b/test/streams/broadcastable_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" require "action_cable" class Turbo::BroadcastableTest < ActionCable::Channel::TestCase diff --git a/test/streams/streams_channel_test.rb b/test/streams/streams_channel_test.rb index 35c990c..dea35c6 100644 --- a/test/streams/streams_channel_test.rb +++ b/test/streams/streams_channel_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" require "action_cable" class Turbo::StreamsChannelTest < ActionCable::Channel::TestCase diff --git a/test/streams/streams_controller_test.rb b/test/streams/streams_controller_test.rb index 733042b..b1f1d3b 100644 --- a/test/streams/streams_controller_test.rb +++ b/test/streams/streams_controller_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" class Turbo::StreamsControllerTest < ActionDispatch::IntegrationTest test "create with respond to" do diff --git a/test/streams/streams_helper_test.rb b/test/streams/streams_helper_test.rb index 8efb764..6ce3643 100644 --- a/test/streams/streams_helper_test.rb +++ b/test/streams/streams_helper_test.rb @@ -1,4 +1,4 @@ -require "turbo_test" +require "test_helper" class TestChannel < ApplicationCable::Channel; end diff --git a/test/system/broadcasts_test.rb b/test/system/broadcasts_test.rb index fab062b..6a2ca3e 100644 --- a/test/system/broadcasts_test.rb +++ b/test/system/broadcasts_test.rb @@ -1,61 +1,62 @@ require "application_system_test_case" class BroadcastsTest < ApplicationSystemTestCase - setup { Message.delete_all } - - include ActionCable::TestHelper - test "Message broadcasts Turbo Streams" do visit messages_path - wait_for_subscriber + subscribe_to_broadcasts - assert_text "Messages" - assert_broadcasts_text "Message 1" do |text| - Message.create(content: text).broadcast_append_to(:messages) + assert_broadcasts_text "Message 1", to: :messages do |text, target| + Message.create(content: text).broadcast_append_to(target) end end - test "New messages update the message count with html" do + test "Message broadcasts with html: render option" do visit messages_path - wait_for_subscriber - - assert_text "Messages" - message = Message.create(content: "A new message") + subscribe_to_broadcasts - message.broadcast_update_to(:messages, target: "message-count", - html: "#{Message.count} messages sent") - assert_selector("#message-count", text: Message.count, wait: 10) + assert_broadcasts_text "Hello, with html: option", to: :messages do |text, target| + Message.create(content: "Ignored").broadcast_append_to(target, html: text) + end end test "Users::Profile broadcasts Turbo Streams" do visit users_profiles_path - wait_for_subscriber + subscribe_to_broadcasts - assert_text "Users::Profiles" - assert_broadcasts_text "Profile 1" do |text| - Users::Profile.new(id: 1, name: text).broadcast_append_to(:users_profiles) + assert_broadcasts_text "Profile 1", to: :users_profiles do |text, target| + Users::Profile.new(id: 1, name: text).broadcast_append_to(target) end end test "passing extra parameters to channel" do visit echo_messages_path - wait_for_subscriber - assert_text "Hello, world!", wait: 100 + assert_broadcasts_text "Hello, world!", to: :messages do + subscribe_to_broadcasts + end end private - def assert_broadcasts_text(text, wait: 5, &block) - assert_no_text text - perform_enqueued_jobs { block.call(text) } - assert_text text, wait: wait + def subscribe_to_broadcasts + click_on "Start listening for broadcasts" + + assert_no_button "Start listening for broadcasts" + + Timeout.timeout(Capybara.default_max_wait_time) { wait_for_subscriber } + end + + def assert_broadcasts_text(text, to:, &block) + within(:element, id: to) { assert_no_text text } + + [text, to].yield_self(&block) + + within(:element, id: to) { assert_text text } end - def wait_for_subscriber(timeout: 10) - time = Time.now + def wait_for_subscriber loop do - subscriber_map = pubsub_adapter.instance_variable_get(:@subscriber_map) + subscriber_map = ActionCable.server.pubsub.instance_variable_get(:@subscriber_map) if subscriber_map.is_a?(ActionCable::SubscriptionAdapter::SubscriberMap) subscribers = subscriber_map.instance_variable_get(:@subscribers) sync = subscriber_map.instance_variable_get(:@sync) @@ -63,9 +64,7 @@ def wait_for_subscriber(timeout: 10) return unless subscribers.empty? end end - assert_operator(Time.now - time, :<, timeout, "subscriber waiting timed out") sleep 0.1 end end - end diff --git a/test/system/form_submissions_test.rb b/test/system/form_submissions_test.rb index f074571..4fa80ec 100644 --- a/test/system/form_submissions_test.rb +++ b/test/system/form_submissions_test.rb @@ -15,7 +15,8 @@ class FormSubmissionsTest < ApplicationSystemTestCase article = Article.create! body: "My article" visit edit_article_path(article.id) - fill_in("Body", with: "My edit").then { click_on "Submit as PATCH" } + fill_in "Body", with: "My edit", fill_options: { clear: :backspace } + click_on "Submit as PATCH" assert_text "Articles" assert_text "My edit", count: 1 diff --git a/test/turbo_test.rb b/test/test_helper.rb similarity index 69% rename from test/turbo_test.rb rename to test/test_helper.rb index a3c070a..bc2878b 100644 --- a/test/turbo_test.rb +++ b/test/test_helper.rb @@ -1,18 +1,15 @@ +# Configure Rails Environment ENV["RAILS_ENV"] = "test" -require "minitest/autorun" -require "rails" +require_relative "../test/dummy/config/environment" +ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] require "rails/test_help" -require "byebug" - -require_relative "dummy/config/environment" -ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] ActionCable.server.config.logger = Logger.new(STDOUT) if ENV["VERBOSE"] module ActionViewTestCaseExtensions - def render(*arguments, **options, &block) - ApplicationController.renderer.render(*arguments, **options, &block) + def render(...) + ApplicationController.renderer.render(...) end end