Update Rakefile, Actions, fix warning #432
Conversation
|
I just noticed #431. Changing this to allow 'Rails main' failures to fail CI is trivial, might be a good idea to leave the code to allow failures in main just in case. |
| run: | | ||
| bin/test test/**/*_test.rb | ||
| id: test | ||
| run: bundle exec rake TESTOPT=-vdc |
There was a problem hiding this comment.
@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:
Lines 4 to 14 in ea00f37
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
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.
A few changes mostly CI oriented, CI will now fail for Rails 6.1 & 7.0 jobs. Rails main is allowed to fail, the easiest way to determine if they are failing is the workflow run page example, the failed jobs are shown in the 'Annotations' section.
Note that Actions does not have an equivalent to 'allow-failure' that is present in Travis & Appveyor.
Commits:
Rakefile - add code from bin/test, fixup rake test - allow running tests from Rake by adding code from bin/test
ci.yml - add more jobs, run tests with rake - tests now log the test name, with failures/errors logged at the end of the log. Rails main jobs are allowed to fail. Also, 'workflow_dispatch' is added, which allows maintainers to run the workflow on a given branch. This can also be used in forks. Can be helpful with intermittent failures...
test/frames/frame_request_controller_test.rb - fix regex warning - fix a regex parameter warning by added parenthesis.
Thought best to separate the commits, assume a `Squash and merge'