Skip to content

Update Rakefile, Actions, fix warning #432

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.

Sign up for GitHub

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
dhh merged 3 commits into from
Feb 20, 2023
Merged

Update Rakefile, Actions, fix warning #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11b9bfa
Rakefile - add code from bin/test, fixup rake test
MSP-Greg Feb 9, 2023
5d62b43
ci.yml - add more jobs, run tests with rake
MSP-Greg Feb 9, 2023
7456f29
test/frames/frame_request_controller_test.rb - fix regex warning
MSP-Greg Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 20 additions & 22 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,44 @@
name: CI
on: [push, pull_request]
on: [push, pull_request, workflow_dispatch]
jobs:
tests:
strategy:
fail-fast: false
matrix:
ruby-version:
- "2.7"
- "3.0"
rails-version:
- "6.1"
- "7.0"
- "main"
rails: [ "6.1", "7.0" ]
ruby: [ "2.7", "3.0", "3.1", "3.2" ]
allow-fail: [ false ]
include:
- { ruby-version: "3.2", rails-version: "7.0" }

# 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
#
# include:
# - ruby-version: head
# continue-on-error: true
- { ruby: "2.7", rails: "main", allow-fail: true }
- { ruby: "3.2", rails: "main", allow-fail: true }
- { ruby: "head", rails: "main", allow-fail: true }

env:
RAILS_VERSION: "${{ matrix.rails-version }}"
RAILS_VERSION: "${{ matrix.rails }}"

name: ${{ format('Tests (Ruby {0}, Rails {1})', matrix.ruby-version, matrix.rails-version) }}
name: ${{ format('Tests (Ruby {0}, Rails {1})', matrix.ruby, matrix.rails) }}
runs-on: ubuntu-latest
continue-on-error: true

steps:
- uses: actions/checkout@v1

- name: Install Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby-version }}
ruby-version: ${{ matrix.ruby }}
rubygems: latest
bundler-cache: true

- name: Run tests
run: |
bin/test test/**/*_test.rb
id: test
run: bundle exec rake TESTOPT=-vdc
Copy link
Contributor

@seanpdoyle seanpdoyle May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MSP-Greg @dhh I believe that this line's change had drastic and unintended side-effects. I believe it's a massive regression in our continuous integration testing harness.

To start, bin/test is responsible for more than just executing the test suite:

turbo-rails/bin/test

Lines 4 to 14 in ea00f37

puts "Installing Ruby dependencies"
`bundle install`
puts "Installing JavaScript dependencies"
`yarn install`
puts "Building JavaScript"
`yarn build`
puts "Preparing test database"
`cd test/dummy; ./bin/rails db:test:prepare`

With the change to rake, CI will no longer:

  • install Yarn dependencies
  • build Turbo's JavaScript
  • execute the System Test suite

Since it won't build the assets, it also fail the build if there are changes to the JavaScript source that are not checked into git.

We should revert this line.

Copy link
Contributor Author

@MSP-Greg MSP-Greg May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Looking at CI, the logs for the test step start with the below, and the tests are logged. I assume I compared log output before and after, but I can't recall.

Also, setup-ruby runs bundle install and also saves/caches the folder, so given some constraints on GitHub Actions caches, CI doesn't need to redownload the gems. I think that was the main reason I made the change, so bundle install wasn't repeated.

Regardless, since I can't recall much about this, and you know this repo much better than I, whatever you think is best...

Installing Ruby dependencies
Installing JavaScript dependencies
Building JavaScript

app/javascript/turbo/index.js → app/assets/javascripts/turbo.js...
created app/assets/javascripts/turbo.js in 665ms

app/javascript/turbo/index.js → app/assets/javascripts/turbo.min.js...
created app/assets/javascripts/turbo.min.js in 1.1s
Preparing test database
Run options: -vdc --seed 63502

Copy link
Contributor Author

@MSP-Greg MSP-Greg May 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I looked at the code a bit more, see the changes below what you commented on, starting with task :test_prereq. At the time, I thought it duplicated the code in bin/test...

Copy link
Contributor

@seanpdoyle seanpdoyle May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MSP-Greg thank you for explaining, and especially for mentioning the :test_prereq task. My apologies: I over-reacted and over-stated my concerns about these changes.

I think I've just internalized the flaky nature of our test suite that I expected pull requests to fail multiple times before passing. When scanning a CI run for #466, I skimmed right past the BroadcastsTest System Tests executing and passing.

Carry on! I'm immensely grateful for whichever changes were responsible for bringing consistency to CI!

Copy link
Contributor Author

@MSP-Greg MSP-Greg May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanpdoyle

I think I've just internalized the flaky nature of our test suite that I expected pull requests to fail multiple times before passing.

Glad I could help with that. Not a good place to be. Failing CI drives me nuts, especially intermittent failures.

I over-reacted and over-stated my concerns about these changes.

Been there, done that. No problem.

continue-on-error: ${{ matrix.allow-fail || false }}

- name: >-
Test outcome: ${{ steps.test.outcome }}
# every step must define a `uses` or `run` key
run: echo NOOP
- name: Fail when generated changes are not checked-in
run: |
git update-index --refresh
Expand Down
17 changes: 15 additions & 2 deletions Rakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,20 @@ load "rails/tasks/statistics.rake"
Rake::TestTask.new do |test|
test.libs << "test"
test.test_files = FileList["test/**/*_test.rb"]
test.warning = false
end

task default: :test
task :test_prereq do
puts "Installing Ruby dependencies"
`bundle install`

puts "Installing JavaScript dependencies"
`yarn install`

puts "Building JavaScript"
`yarn build`

puts "Preparing test database"
`cd test/dummy; ./bin/rails db:test:prepare; cd ../..`
end

task default: [:test_prereq, :test]
4 changes: 2 additions & 2 deletions test/frames/frame_request_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ class Turbo::FrameRequestControllerTest < ActionDispatch::IntegrationTest
turbo_frame_request_id = "test_frame_id"

get tray_path(id: 1)
assert_no_match /#{turbo_frame_request_id}/, @response.body
assert_no_match(/#{turbo_frame_request_id}/, @response.body)

get tray_path(id: 1), headers: { "Turbo-Frame" => turbo_frame_request_id }
assert_match /#{turbo_frame_request_id}/, @response.body
assert_match(/#{turbo_frame_request_id}/, @response.body)
end

private
Expand Down