Skip to content

Commit

Permalink
Raise an error if PERCY_TOKEN is set but PERCY_PROJECT is not.
Browse files Browse the repository at this point in the history
  • Loading branch information
@fotinakis
fotinakis committed Jan 7, 2017
1 parent 6bafaf8 commit e0f019b
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 1 deletion.
7 changes: 7 additions & 0 deletions lib/percy/capybara/client.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ def initialize(options = {})

# Check that environment variables required for Percy::Client are set
def required_environment_variables_set?
if !ENV['PERCY_TOKEN'].nil? && ENV['PERCY_PROJECT'].nil?
raise RuntimeError.new(
'[percy] It looks like you were trying to enable Percy because PERCY_TOKEN is set, ' +
'but you are missing the PERCY_PROJECT environment variable!'
)
end

!(ENV['PERCY_PROJECT'].nil? || ENV['PERCY_TOKEN'].nil?)
end

Expand Down
2 changes: 2 additions & 0 deletions lib/percy/capybara/rspec.rb
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
require 'rspec/core'

# TODO: this has been deprecated for a long time, remove this file when releasing v3.0.0.

RSpec.configure do |config|
config.before(:suite) { Percy::Capybara.initialize_build }
config.after(:suite) { Percy::Capybara.finalize_build }
Expand Down
6 changes: 5 additions & 1 deletion spec/lib/percy/capybara/client_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
context 'when required environment variables set' do
before(:context) { set_required_env_variables }
after(:context) { clear_required_env_variables }

it 'is true when PERCY_ENABLE is 1' do
ENV['PERCY_ENABLE'] = '1'
expect(Percy::Capybara::Client.new.enabled?).to eq(true)
Expand All @@ -21,6 +21,10 @@
ENV['PERCY_ENABLE'] = '0'
expect(Percy::Capybara::Client.new.enabled?).to eq(false)
end
it 'raises error if PERCY_TOKEN is set but PERCY_PROJECT is not' do
ENV.delete 'PERCY_PROJECT'
expect { Percy::Capybara::Client.new.enabled? }.to raise_error(RuntimeError)
end
end

context 'when required environment variables not set' do
Expand Down

6 comments on commit e0f019b

@gareth
Copy link

@gareth gareth commented on e0f019b Mar 24, 2017 •
edited

Choose a reason for hiding this comment

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

Does the PERCY_TOKEN variable specifically need to be set if the access_token is manually configured as your documentation describes? This feels like it would raise errors in those cases where it shouldn't.

Maybe I'm missing something about the different use cases for configuring Percy. I know it has to be flexible enough to work in various environments and continuous integration suites so I probably don't have all the context.

@timhaines
Copy link
Contributor

Choose a reason for hiding this comment

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

@gareth If you're using percy-capybara, it's currently not going to work without the environment variables set (the docs you linked to are for percy-client, a different gem, that needs a docs update).

Can you explain a little about your use case, describing the scenario where setting an environment variable doesn't work well for you please? I'd like to understand it better.

@fotinakis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timhaines There is another way to set the token in code by doing Percy.config.access_token = 'foo'. That's what @gareth is referencing.

@gareth You bring up a good point, the current state of the gem requires the ENV vars to be set when really we should be flexible on that. It was just an oversight, not a specific design decision. Any chance you're interested in sending up a PR for that use case? Otherwise we're happy to take a look, would be great to get an issue filed to remind us.

@gareth
Copy link

@gareth gareth commented on e0f019b Mar 24, 2017

Choose a reason for hiding this comment

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

Thanks for the quick replies, Tim and Mike. I'm happy to open an issue but I think I'd have to fiddle around more to be able to write a PR - as I mentioned it would take more knowledge of Percy's internals to understand the implications. I might try over the weekend though

@fotinakis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gareth no prob :) if this is blocking you from upgrading, I'm happy to take care of it quickly. Let me know what you think.

@gareth
Copy link

@gareth gareth commented on e0f019b Mar 24, 2017

Choose a reason for hiding this comment

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

No, you chatted to @samjewell the other day and helped resolve the problem we were having. This issue was just something we discovered while post-mortem-ing the situation, trying to work out what specific dependencies the library had at different points. It's not blocking us at all

Please sign in to comment.