Preserve ENV["TESTOPTS"] when verbose is enabled#723
Merged
Conversation
Rake 13.4.0 made `FileUtilsExt.verbose(true)` set `ENV["TESTOPTS"] = "-v"` unconditionally so that `rake -v test` would propagate verbosity to the test runner. The unconditional assignment also overwrote values set by other tools such as `ci_reporter_minitest`, breaking any workflow that relied on `TESTOPTS` being populated before `TestTask` ran. Move the `-v` injection out of `FileUtilsExt.verbose` and into `Rake::TestTask#option_list`. The task block now computes an effective verbose flag from `@verbose` or the global `FileUtilsExt.verbose_flag`, passes it to `option_list(verbose:)`, and the option list appends `-v` to the existing `TESTOPTS` only when it is not already present. This keeps the PR #394 behavior for `rake -v` and `t.verbose = true` while no longer clobbering pre-set `TESTOPTS` values. Fixes #722. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where enabling verbose mode (rake -v / t.verbose = true) could clobber a pre-existing ENV["TESTOPTS"] (e.g., set by ci_reporter_minitest). It does this by removing TESTOPTS mutation from FileUtilsExt.verbose and instead computing/appending -v within Rake::TestTask’s option handling.
Changes:
- Stop setting
ENV["TESTOPTS"]insideRake::FileUtilsExt.verbose. - Compute an “effective verbose” flag in
Rake::TestTask#define(task-local verbose OR global-v) and pass it intooption_list. - Update/add tests to validate
option_listappends-vwithout overwriting existingTESTOPTS.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lib/rake/testtask.rb |
Moves -v injection into option_list(verbose:) and uses an effective verbose flag during task execution. |
lib/rake/file_utils_ext.rb |
Removes the unconditional ENV["TESTOPTS"] = "-v" side effect from verbose. |
test/test_rake_test_task.rb |
Adds coverage for option_list behavior with/without TESTOPTS and duplicate -v handling. |
test/test_rake_file_utils.rb |
Removes assertions/cleanup tied to FileUtilsExt.verbose mutating TESTOPTS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+194
to
+196
| def test_option_list_verbose_without_testopts | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "-v", tt.option_list |
There was a problem hiding this comment.
test_option_list_verbose_without_testopts assumes ENV["TESTOPTS"] is unset; if a developer/CI environment has TESTOPTS pre-populated, this assertion will fail. Consider saving the original value and clearing/restoring TESTOPTS within this test (similar to the other tests here).
Suggested change
| def test_option_list_verbose_without_testopts | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v", tt.option_list | |
| def test_option_list_verbose_without_testopts | |
| testopts = ENV["TESTOPTS"] | |
| ENV.delete "TESTOPTS" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v", tt.option_list | |
| ensure | |
| if testopts | |
| ENV["TESTOPTS"] = testopts | |
| else | |
| ENV.delete "TESTOPTS" | |
| end |
Comment on lines
+200
to
+220
| ENV["TESTOPTS"] = "--ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "--ci-reporter -v", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" | ||
| end | ||
|
|
||
| def test_option_list_not_verbose_with_testopts | ||
| ENV["TESTOPTS"] = "--ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = false } | ||
| assert_equal "--ci-reporter", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" | ||
| end | ||
|
|
||
| def test_option_list_skips_duplicate_v | ||
| ENV["TESTOPTS"] = "-v --ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "-v --ci-reporter", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" |
There was a problem hiding this comment.
This test unconditionally ENV.delete "TESTOPTS" in ensure. If TESTOPTS was already set in the process environment, this will clobber it for subsequent tests. Prefer capturing the previous value and restoring it in ensure (set it back or delete only if it was originally absent).
Suggested change
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "--ci-reporter -v", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| end | |
| def test_option_list_not_verbose_with_testopts | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = false } | |
| assert_equal "--ci-reporter", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| end | |
| def test_option_list_skips_duplicate_v | |
| ENV["TESTOPTS"] = "-v --ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v --ci-reporter", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| old_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "--ci-reporter -v", tt.option_list | |
| ensure | |
| if old_testopts | |
| ENV["TESTOPTS"] = old_testopts | |
| else | |
| ENV.delete "TESTOPTS" | |
| end | |
| end | |
| def test_option_list_not_verbose_with_testopts | |
| old_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = false } | |
| assert_equal "--ci-reporter", tt.option_list | |
| ensure | |
| if old_testopts | |
| ENV["TESTOPTS"] = old_testopts | |
| else | |
| ENV.delete "TESTOPTS" | |
| end | |
| end | |
| def test_option_list_skips_duplicate_v | |
| old_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "-v --ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v --ci-reporter", tt.option_list | |
| ensure | |
| if old_testopts | |
| ENV["TESTOPTS"] = old_testopts | |
| else | |
| ENV.delete "TESTOPTS" | |
| end |
Comment on lines
+200
to
+220
| ENV["TESTOPTS"] = "--ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "--ci-reporter -v", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" | ||
| end | ||
|
|
||
| def test_option_list_not_verbose_with_testopts | ||
| ENV["TESTOPTS"] = "--ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = false } | ||
| assert_equal "--ci-reporter", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" | ||
| end | ||
|
|
||
| def test_option_list_skips_duplicate_v | ||
| ENV["TESTOPTS"] = "-v --ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "-v --ci-reporter", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" |
There was a problem hiding this comment.
This test unconditionally ENV.delete "TESTOPTS" in ensure. If TESTOPTS was already set in the process environment, this will clobber it for subsequent tests. Prefer capturing the previous value and restoring it in ensure (set it back or delete only if it was originally absent).
Suggested change
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "--ci-reporter -v", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| end | |
| def test_option_list_not_verbose_with_testopts | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = false } | |
| assert_equal "--ci-reporter", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| end | |
| def test_option_list_skips_duplicate_v | |
| ENV["TESTOPTS"] = "-v --ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v --ci-reporter", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| previous_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "--ci-reporter -v", tt.option_list | |
| ensure | |
| if previous_testopts.nil? | |
| ENV.delete "TESTOPTS" | |
| else | |
| ENV["TESTOPTS"] = previous_testopts | |
| end | |
| end | |
| def test_option_list_not_verbose_with_testopts | |
| previous_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = false } | |
| assert_equal "--ci-reporter", tt.option_list | |
| ensure | |
| if previous_testopts.nil? | |
| ENV.delete "TESTOPTS" | |
| else | |
| ENV["TESTOPTS"] = previous_testopts | |
| end | |
| end | |
| def test_option_list_skips_duplicate_v | |
| previous_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "-v --ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v --ci-reporter", tt.option_list | |
| ensure | |
| if previous_testopts.nil? | |
| ENV.delete "TESTOPTS" | |
| else | |
| ENV["TESTOPTS"] = previous_testopts | |
| end |
Comment on lines
+200
to
+220
| ENV["TESTOPTS"] = "--ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "--ci-reporter -v", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" | ||
| end | ||
|
|
||
| def test_option_list_not_verbose_with_testopts | ||
| ENV["TESTOPTS"] = "--ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = false } | ||
| assert_equal "--ci-reporter", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" | ||
| end | ||
|
|
||
| def test_option_list_skips_duplicate_v | ||
| ENV["TESTOPTS"] = "-v --ci-reporter" | ||
| tt = Rake::TestTask.new { |t| t.verbose = true } | ||
| assert_equal "-v --ci-reporter", tt.option_list | ||
| ensure | ||
| ENV.delete "TESTOPTS" |
There was a problem hiding this comment.
This test unconditionally ENV.delete "TESTOPTS" in ensure. If TESTOPTS was already set in the process environment, this will clobber it for subsequent tests. Prefer capturing the previous value and restoring it in ensure (set it back or delete only if it was originally absent).
Suggested change
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "--ci-reporter -v", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| end | |
| def test_option_list_not_verbose_with_testopts | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = false } | |
| assert_equal "--ci-reporter", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| end | |
| def test_option_list_skips_duplicate_v | |
| ENV["TESTOPTS"] = "-v --ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v --ci-reporter", tt.option_list | |
| ensure | |
| ENV.delete "TESTOPTS" | |
| original_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "--ci-reporter -v", tt.option_list | |
| ensure | |
| if original_testopts.nil? | |
| ENV.delete "TESTOPTS" | |
| else | |
| ENV["TESTOPTS"] = original_testopts | |
| end | |
| end | |
| def test_option_list_not_verbose_with_testopts | |
| original_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "--ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = false } | |
| assert_equal "--ci-reporter", tt.option_list | |
| ensure | |
| if original_testopts.nil? | |
| ENV.delete "TESTOPTS" | |
| else | |
| ENV["TESTOPTS"] = original_testopts | |
| end | |
| end | |
| def test_option_list_skips_duplicate_v | |
| original_testopts = ENV["TESTOPTS"] | |
| ENV["TESTOPTS"] = "-v --ci-reporter" | |
| tt = Rake::TestTask.new { |t| t.verbose = true } | |
| assert_equal "-v --ci-reporter", tt.option_list | |
| ensure | |
| if original_testopts.nil? | |
| ENV.delete "TESTOPTS" | |
| else | |
| ENV["TESTOPTS"] = original_testopts | |
| end |
Copilot review on #723 pointed out that the new option_list tests either assumed TESTOPTS was unset or unconditionally deleted it in ensure, which would clobber a pre-existing TESTOPTS and leak state across tests. Save and restore the previous value from setup/teardown so each test starts from a clean slate without losing the process environment value, and drop the now-redundant ensure blocks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or 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
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.
Rake 13.4.0 made
FileUtilsExt.verbose(true)setENV["TESTOPTS"] = "-v"unconditionally so thatrake -v testwould propagate verbosity to the test runner. The unconditional assignment also overwrote values set by other tools such asci_reporter_minitest, breaking any workflow that relied onTESTOPTSbeing populated beforeTestTaskran.Move the
-vinjection out ofFileUtilsExt.verboseand intoRake::TestTask#option_list. The task block now computes an effective verbose flag from@verboseor the globalFileUtilsExt.verbose_flag, passes it tooption_list(verbose:), and the option list appends-vto the existingTESTOPTSonly when it is not already present. This keeps the PR #394 behavior forrake -vandt.verbose = truewhile no longer clobbering pre-setTESTOPTSvalues.Fixes #722.