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

Add additional config to docs about how to use remote browser in test #44981

Conversation

tan-linx
Copy link
Contributor

@tan-linx tan-linx commented Apr 29, 2022

Adds additional config to docs about how to use remote browser in test (for Docker, Selenium Grid, CI, etc.). We need this config for testing the upload of files with Active Storage. Closes #43682, #47706

@rails-bot rails-bot bot added the docs label Apr 29, 2022
@viktorianer
Copy link
Contributor

Hey @p8 or @vipulnsward, could you have a look at this improvement, please?

Comment on lines +872 to +875
options = {
browser: ENV["SELENIUM_REMOTE_URL"].blank? ? :chrome : :remote,
url: ENV["SELENIUM_REMOTE_URL"].blank? ? nil : ENV["SELENIUM_REMOTE_URL"]
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's clearer if we just use an if-statement:

Suggested change
options = {
browser: ENV["SELENIUM_REMOTE_URL"].blank? ? :chrome : :remote,
url: ENV["SELENIUM_REMOTE_URL"].blank? ? nil : ENV["SELENIUM_REMOTE_URL"]
}
options = if ENV["SELENIUM_REMOTE_URL"].present?
{ browser: :remote, url: ENV["SELENIUM_REMOTE_URL"] }
else
{ browser: :chrome }
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's clearer if we just use an if-statement:

Good idea. What abbaut using present?? We had cases, where the url was just "" and the error message did not help to figure out this issue. I think it is safer to use blank? here.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can also use ENV.fetch with a default instead of multiple if/else:
https://docs.ruby-lang.org/en/master/ENV.html#method-c-fetch

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, together it would be:

url = ENV.fetch("SELENIUM_REMOTE_URL", nil)
options = if url
    { browser: :remote,  url }
  else
    { browser: :chrome } 
  end

Can we create a new PR, as @tan-linx is not responding :)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants