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

remove unnessary option setting from test runner #24696

Merged
merged 2 commits into from
Apr 25, 2016

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Apr 23, 2016

If run the test over the rake command, because of the test patterns is passed
via rake_run method, do not need to be obtained from the argv.

This probably fixes #24372.

@rails-bot
Copy link

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)


assert_match '1 runs, 1 assertions', output
assert_match 'Execute test', output
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we add test from #24372 to accompany this?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean add a test using --rakefile option?

If so, I've added a test.

@vipulnsward
Copy link
Member

Nice fix. This does work for me now:

rake --rakefile "`pwd`/Rakefile" test
bundle exec rake --rakefile "`pwd`/Rakefile" test

But

rake --rakefile "`pwd`/Rakefile" routes

doesn't. We can probably check and include a fix for that here.

@y-yagi y-yagi force-pushed the remove_unnessary_option_setting branch from dacb714 to 1c52de9 Compare April 24, 2016 04:53
@y-yagi
Copy link
Member Author

y-yagi commented Apr 24, 2016

Thank you for pointing it out!

Also fixed routes task.

@@ -19,6 +19,9 @@ task routes: :environment do

OptionParser.new do |opts|
opts.banner = "Usage: rails routes [options]"

Rake.application.standard_rake_options.each { |args| opts.on(*args) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is iffy. We'd likely need to swallow rake options for every Rails command with custom option parsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, come to think of it there might not be other cases since routes and test are the only rake tasks we fiddled with in Rails 5. So this feels like an ok compromise 👍

If run the test over the `rake` command, because of the test patterns is passed
via `rake_run` method, do not need to be obtained from the argv.

This probably fixes rails#24372.
In order to prevent `OptionParser::ParseError` when specify the rake options to
`routes` task.
@y-yagi y-yagi force-pushed the remove_unnessary_option_setting branch from 1c52de9 to 996092e Compare April 24, 2016 23:38
@kaspth kaspth merged commit e007afd into rails:master Apr 25, 2016
@kaspth
Copy link
Contributor

kaspth commented Apr 25, 2016

❤️

@y-yagi
Copy link
Member Author

y-yagi commented Apr 25, 2016

😄

@y-yagi y-yagi deleted the remove_unnessary_option_setting branch April 25, 2016 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Valid rake command line arguments reported as invalid on running 'test' task
5 participants