Improve upon test suite flakiness (#327)

seanpdoyle · web-flow · commit f9872c35fdcd · 2022-08-02T14:24:27.000-04:00
* 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. > > - [#248][] As a follow-up to the [short-term solution][] shipped in [#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 [#248]: #248 [short-term solution]: c2dc5b1 * 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 <top (required)> 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 <top (required)> 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 `<turbo-stream-source>` element. Instead, append the element when clicking a `<button>`.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -11,7 +11,6 @@ jobs:
           - "6.1"
           - "7.0"
           - "main"
-        continue-on-error: [ false ]
 
         # Disabled until minitest relaxes its upper bound: https://github.com/seattlerb/minitest/pull/862
         # > minitest-5.14.2 requires ruby version < 3.1, >= 2.2, which is incompatible with the current version, ruby 3.1.0p-1
@@ -25,7 +24,7 @@ jobs:
 
     name: ${{ format('Tests (Ruby {0}, Rails {1})', matrix.ruby-version, matrix.rails-version) }}
     runs-on: ubuntu-latest
-    continue-on-error: ${{ matrix.continue-on-error }}
+    continue-on-error: true
 
     steps:
       - uses: actions/checkout@v1
diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb
@@ -1,4 +1,4 @@
-require "turbo_test"
+require "test_helper"
 
 class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
   driven_by :selenium, using: :headless_chrome, screen_size: [1400, 1400]
diff --git a/test/drive/drive_helper_test.rb b/test/drive/drive_helper_test.rb
@@ -1,4 +1,4 @@
-require "turbo_test"
+require "test_helper"
 
 class Turbo::DriveHelperTest < ActionDispatch::IntegrationTest
   test "opting out of the default cache" do
diff --git a/test/dummy/app/views/application/_template_button.html.erb b/test/dummy/app/views/application/_template_button.html.erb
@@ -0,0 +1,14 @@
+<button type="button">
+  <%= content %>
+
+  <script>
+    document.currentScript.parentElement.addEventListener("click", ({ currentTarget }) => {
+      for (const template of currentTarget.querySelectorAll("template")) {
+        currentTarget.parentElement.insertBefore(template.content, currentTarget)
+      }
+      currentTarget.remove()
+    })
+  </script>
+
+  <template><%= yield %></template>
+</button>
diff --git a/test/dummy/app/views/messages/echo.html.erb b/test/dummy/app/views/messages/echo.html.erb
@@ -1,5 +1,8 @@
 <h1>Echo Messages</h1>
 
-<%= 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 %>
+
 <div id="messages">
 </div>
diff --git a/test/dummy/app/views/messages/index.html.erb b/test/dummy/app/views/messages/index.html.erb
@@ -1,9 +1,8 @@
 <h1>Messages</h1>
 
-<span id="message-count">
-  <%= @messages.count %> messages sent
-</span>
+<%= render "template_button", content: "Start listening for broadcasts" do %>
+  <%= turbo_stream_from "messages" %>
+<% end %>
 
-<%= turbo_stream_from "messages" %>
 <div id="messages">
 </div>
diff --git a/test/dummy/app/views/users/profiles/index.html.erb b/test/dummy/app/views/users/profiles/index.html.erb
@@ -1,4 +1,8 @@
 <h1>Users::Profiles</h1>
 
-<%= turbo_stream_from "users_profiles" %>
-<div id="users_profiles"></div>
+<%= render "template_button", content: "Start listening for broadcasts" do %>
+  <%= turbo_stream_from "users_profiles" %>
+<% end %>
+
+<div id="users_profiles">
+</div>
diff --git 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
@@ -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
@@ -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
@@ -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 src="/trays/1" id="tray"></turbo-frame>), turbo_frame_tag("tray", src: "/trays/1")
   end
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,71 +1,70 @@
 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)
         sync.synchronize do
           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
@@ -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/test_helper.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
 

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-require "turbo_test"`
	`1`	`+require "test_helper"`
`2`	`2`
`3`	`3`	`class ApplicationSystemTestCase < ActionDispatch::SystemTestCase`
`4`	`4`	`driven_by :selenium, using: :headless_chrome, screen_size: [1400, 1400]`