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

Improve upon test suite flakiness #327

Merged
merged 6 commits into from
Aug 2, 2022
Merged

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented May 3, 2022

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.

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

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 Broadcast tests

First, don't render HTML with the <turbo-cable-stream-source> element.
Instead, append the element when clicking a <button>.

Next, remove all waiting from Capybara tests, and rely on default
values.

@seanpdoyle seanpdoyle changed the title Use Ruby 2.7 argument forwarding Address Test Suite Deprecation Warnings May 3, 2022
@dhh
Copy link
Member

dhh commented May 3, 2022

Dig that. Seems like we have some test errors, but maybe that's related to this other PR I just merged. Looks like those tests might be flaky?

@seanpdoyle
Copy link
Contributor Author

The current batch of CI failures here (and in #328) seem related to test/system/broadcasts_test.rb, which was recently introduced in #308.

@dhh
Copy link
Member

dhh commented May 3, 2022

Cc @yrashk

@dhh
Copy link
Member

dhh commented Jun 19, 2022

Seems like we still have some Rails 6.1 failures?

> 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#248][]

As a follow-up to the [short-term solution][] shipped in
[hotwired#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#248]: hotwired#248
[short-term solution]: hotwired@c2dc5b1
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
```
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.
Remove the `continue-on-error` configuration and instead allow other
jobs complete in spite of failures.
@seanpdoyle seanpdoyle force-pushed the deprecations branch 2 times, most recently from b3180b3 to 1fece02 Compare August 1, 2022 20:30
@seanpdoyle seanpdoyle changed the title Address Test Suite Deprecation Warnings Improve upon test suite flakiness Aug 1, 2022
@seanpdoyle seanpdoyle force-pushed the deprecations branch 9 times, most recently from e91d3db to f2ca315 Compare August 2, 2022 14:20
Resolve a flaky System Test by ensuring that an input is clear before
filling in a new value.
@seanpdoyle seanpdoyle force-pushed the deprecations branch 4 times, most recently from 2d9a6b3 to 89e508c Compare August 2, 2022 15:29
First, don't render HTML with the `<turbo-stream-source>` element.
Instead, append the element when clicking a `<button>`.
@seanpdoyle
Copy link
Contributor Author

This is passing, finally.

@dhh dhh merged commit f9872c3 into hotwired:main Aug 2, 2022
@seanpdoyle seanpdoyle deleted the deprecations branch August 2, 2022 18:32
dhh added a commit that referenced this pull request Aug 3, 2022
* main:
  Improve upon test suite flakiness (#327)
  Give html: rendering param the same treatment as content: (#362)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants