Skip to content

Removes noisy puts statement#975

Merged
monfresh merged 1 commit intomasterfrom
amos-fix-rspec_warning_output
Jan 26, 2017
Merged

Removes noisy puts statement#975
monfresh merged 1 commit intomasterfrom
amos-fix-rspec_warning_output

Conversation

@amoose
Copy link
Copy Markdown
Contributor

@amoose amoose commented Jan 20, 2017

Why

We have a special rake task for running user flow specs, but only when RSpec is present.

How

Removes noisy puts statement and fails silently since Rails.logger isn't available.

@monfresh
Copy link
Copy Markdown
Contributor

LGTM, but out of curiosity, in what scenario would RSpec not be present?

@monfresh
Copy link
Copy Markdown
Contributor

And if RSpec is really not present, why fail silently? If the rake task depends on RSpec, don't we want to let the LoadError be raised?

@amoose
Copy link
Copy Markdown
Contributor Author

amoose commented Jan 24, 2017

RSpec is not present in production; where the bundle is without development, test, etc. We do not want to raise the exception because this rake task is only meant to be available in the development/test environments. It will only work, in fact, if the test group is included.

@amoose
Copy link
Copy Markdown
Contributor Author

amoose commented Jan 24, 2017

I guess the other option would be to put this entire task in a big conditional? What do you think the best way to handle this is?

@amoose amoose self-assigned this Jan 24, 2017
@amoose amoose requested a review from monfresh January 24, 2017 23:40
@monfresh
Copy link
Copy Markdown
Contributor

I'm still not quite sure what the problem we're trying to solve is. If someone attempts to run the rake task in production, raising an error seems reasonable to me since it's not meant to be run in production. Is the issue that the rake task somehow is getting invoked automatically when deploying or cooking our servers?

@monfresh
Copy link
Copy Markdown
Contributor

I get it now. Whenever rake is invoked, it loads all rake tasks, and triggers the rescue in production. I would suggest wrapping the task in a conditional, like you suggested, and removing the rescue:

unless Rails.env.production?
  namespace :spec do
    desc 'Executes user flow specs'
    RSpec::Core::RakeTask.new('user_flows') do |t|
      t.rspec_opts = %w[--tag user_flow
                        --order defined
                        --require ./lib/rspec/formatters/user_flow_formatter.rb
                        --format UserFlowFormatter]
    end
  end
end

@monfresh
Copy link
Copy Markdown
Contributor

Pinging @amoose. If you're busy with other things, do you want me to make the change I recommended so we can merge this?

@amoose
Copy link
Copy Markdown
Contributor Author

amoose commented Jan 26, 2017

Yes, that would be great Moncef, thanks!

@monfresh monfresh force-pushed the amos-fix-rspec_warning_output branch from 0a5c32e to c4cb69c Compare January 26, 2017 21:33
Copy link
Copy Markdown
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

LGTM

**Why**: When the `rake` command is invoked, it parses all rake tasks,
even if they're not being called directly. The user_flows rake task
requires RSpec, and is not meant to be run in production. By ignoring
production, we suppress error messages due to RSpec not being present,
which aren't relevant in production.

In development, we don't want to rescue the LoadError. We want to make
sure the rake task aborts because if RSpec is not present, it's a
problem.
@monfresh monfresh force-pushed the amos-fix-rspec_warning_output branch from c4cb69c to c3f1c07 Compare January 26, 2017 22:22
@monfresh monfresh merged commit 24b4f02 into master Jan 26, 2017
@monfresh monfresh deleted the amos-fix-rspec_warning_output branch January 26, 2017 22:51
amoose added a commit that referenced this pull request Mar 7, 2017
**Why**: When the `rake` command is invoked, it parses all rake tasks,
even if they're not being called directly. The user_flows rake task
requires RSpec, and is not meant to be run in production. By ignoring
production, we suppress error messages due to RSpec not being present,
which aren't relevant in production.

In development, we don't want to rescue the LoadError. We want to make
sure the rake task aborts because if RSpec is not present, it's a
problem.
amoose added a commit that referenced this pull request Mar 8, 2017
**Why**: When the `rake` command is invoked, it parses all rake tasks,
even if they're not being called directly. The user_flows rake task
requires RSpec, and is not meant to be run in production. By ignoring
production, we suppress error messages due to RSpec not being present,
which aren't relevant in production.

In development, we don't want to rescue the LoadError. We want to make
sure the rake task aborts because if RSpec is not present, it's a
problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants