🤫 Quiet down railties tests #49679
Merged
🤫 Quiet down railties tests #49679
+164
−201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skipkayhil
reviewed
Oct 17, 2023
railties/test/application/action_controller_test_case_integration_test.rb
Outdated
Show resolved
Hide resolved
zzak
commented
Oct 18, 2023
matthewd
reviewed
Oct 18, 2023
matthewd
reviewed
Oct 18, 2023
railties/test/application/action_controller_test_case_integration_test.rb
Outdated
Show resolved
Hide resolved
zzak
force-pushed
the
railties-quiet
branch
8 times, most recently
from
f0c5ac3
to
66556bc
Compare
Comment on lines
731
to
830
| command_check = -> command, *_ do | ||
| if command == "importmap:install" | ||
| flunk "`importmap:install` expected to not be called." | ||
| end | ||
| end | ||
| assert_nil @rails_commands, "`importmap:install` expected to not be called." | ||
|
|
||
| generator.stub(:rails_command, command_check) do | ||
| run_generator_instance | ||
| end | ||
| run_generator_instance |
There was a problem hiding this comment.
Here, you are asserting before running the generator, so @rails_commands will always be nil.
There was a problem hiding this comment.
@jonathanhefner Nice catch!
I thought about this, but can you think of an elegant way to assert neither of these commands are called?
--- a/railties/test/generators/app_generator_test.rb
+++ b/railties/test/generators/app_generator_test.rb
@@ -728,10 +728,11 @@ def test_skip_active_job_option
def test_skip_javascript_option
generator([destination_root], skip_javascript: true)
- assert_nil @rails_commands, "`importmap:install` expected to not be called."
-
run_generator_instance
+ assert_not_includes @rails_commands, "importmap:install", "`importmap:install` expected to not be called."
+ assert_not_includes @rails_commands, "turbo:install stimulus:install", "`turbo:install stimulus:install` expected to not be called."
Comment on lines
892
to
1004
| def test_default_generator_executes_all_rails_commands | ||
| run_generator_and_bundler [destination_root] | ||
|
|
||
| expected_commands = [ | ||
| "credentials:diff --enroll", "importmap:install", "turbo:install stimulus:install" | ||
| ] | ||
| assert_equal expected_commands, @rails_commands | ||
| end |
There was a problem hiding this comment.
I don't think this test is necessary. test_generator_configures_decrypted_diffs_by_default already asserts the result of credentials:diff --enroll, and the tests you've modified cover the other commands. Additionally, this test is brittle. We would have to update it whenever we add or remove and rails_command calls in the app generator.
There was a problem hiding this comment.
The idea was to make sure that inserting a random rails_command "lol" somewhere will trip something somewhere in the test suite, and not only when you run it live, same goes for the empty test case.
zzak
force-pushed
the
railties-quiet
branch
2 times, most recently
from
6b79ec5
to
81350b3
Compare
zzak
added a commit
to zzak/stimulus-rails
that referenced
this pull request
Jan 11, 2024
This commit adds test coverage for the installer Rake task and application template. The installer is run against a freshly generated Rails app using the version of Rails that is currently loaded. Thus the installer can be tested with different versions of Rails in CI. Similar to rails/importmap-rails#206 for stimulus-rails The motivation is to be able to move these tests from railties, see: rails/rails#49679 Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
zzak
added a commit
to zzak/stimulus-rails
that referenced
this pull request
Jan 11, 2024
This commit adds test coverage for the installer Rake task and application template. The installer is run against a freshly generated Rails app using the version of Rails that is currently loaded. Thus the installer can be tested with different versions of Rails in CI. Similar to rails/importmap-rails#206 for stimulus-rails The motivation is to be able to move these tests from railties, see: rails/rails#49679 Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
zzak
added a commit
to zzak/turbo-rails
that referenced
this pull request
Jan 11, 2024
This commit adds test coverage for the installer Rake task and application template. The installer is run against a freshly generated Rails app using the version of Rails that is currently loaded. Thus the installer can be tested with different versions of Rails in CI. Similar to hotwired/stimulus-rails#136. The motivation is to be able to move these tests from railties, see: rails/rails#49679 Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
zzak
added a commit
to zzak/turbo-rails
that referenced
this pull request
Apr 19, 2024
This commit adds test coverage for the installer Rake task and application template. The installer is run against a freshly generated Rails app using the version of Rails that is currently loaded. Thus the installer can be tested with different versions of Rails in CI. Similar to hotwired/stimulus-rails#136. The motivation is to be able to move these tests from railties, see: rails/rails#49679 Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
zzak
force-pushed
the
railties-quiet
branch
2 times, most recently
from
5ff74e6
to
01bf7c1
Compare
When rails#48269 was merged any gem installed during `rails new` which calls `app:template` would cause the install command to be executed and consequently `bundle install` would also run. We want to avoid running these commands in our tests because they are very expensive. It is up to the gem (importmap, etc) to test the behavior of the install command, not railties. Before ``` $ bin/test test/generators/plugin_generator_test.rb test/generators/app_generator_test.rb Finished in 320.803659s, 0.8541 runs/s, 7.1913 assertions/s. 274 runs, 2307 assertions, 14 failures, 0 errors, 0 skips ``` After ``` Finished in 70.316250s, 3.9251 runs/s, 34.3164 assertions/s. 276 runs, 2413 assertions, 0 failures, 0 errors, 0 skips ```
The default of :info generates a ton of unnecessary noise in the railties logs.
Since we are stubbing bundle commands, we can avoid sprockets side-effects here. This commit also fixes the tests for preservation of sprockets during `app:update`. Due to propshaft being the default, this test was unnecessary.
This will make sure we have less tests to run since those are slow. Generate the app once and run many assertions on it instead of generating the same app multiple times.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addressed several unrelated issues in the Railties test suite, in order to reduce the amount of noise.
After
Before