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.

Sign up for GitHub

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

Ew replace phantomjs with selenium 2 #1718

Merged
merged 10 commits into from
Jul 30, 2020
Merged

Ew replace phantomjs with selenium 2 #1718

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
da04053
Merge pull request #1 from thoughtbot/master
EWright212 Jul 6, 2020
ab44160
Replaced phantomjs with selenium in gemfile and rails_helper, as phan…
EWright212 Jul 21, 2020
f749d31
Updated failing tests due to replacing phantomjs with selenium webdriver
EWright212 Jul 21, 2020
769b550
Refactor - fix houndbot linting warnings
EWright212 Jul 21, 2020
5a2aee8
Refactor - fix houndbot linting warnings
EWright212 Jul 21, 2020
65e9b3c
Refactor remove single quotes
EWright212 Jul 24, 2020
f7833f6
Update spec/features/orders_index_spec.rb
EWright212 Jul 24, 2020
aaeceed
modify git ignore to keep tmp pids .keep which enables local server h…
EWright212 Jul 29, 2020
a9742ef
modify git ignore to keep tmp pids .keep which enables local server h…
EWright212 Jul 29, 2020
7178905
Refactor fixing erronious houndbot warnings
EWright212 Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
/spec/example_app/public/assets
/tags
/tmp/*
!/tmp/pids/.keep
/spec/example_app/tmp/*
pkg
*.ipr
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ group :test do
gem "database_cleaner"
gem "formulaic"
gem "launchy"
gem "poltergeist"
gem "pundit"
gem 'selenium-webdriver'
nickcharlton marked this conversation as resolved.
Show resolved Hide resolved
gem "shoulda-matchers"
gem "timecop"
gem "webmock"
Expand Down
15 changes: 6 additions & 9 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ GEM
rack-test (>= 0.6.3)
regexp_parser (~> 1.5)
xpath (~> 3.2)
cliver (0.3.2)
childprocess (3.0.0)
coderay (1.1.2)
concurrent-ruby (1.1.6)
crack (0.4.3)
Expand Down Expand Up @@ -150,10 +150,6 @@ GEM
parser (2.7.0.4)
ast (~> 2.4.0)
pg (1.2.3)
poltergeist (1.18.1)
capybara (>= 2.1, < 4)
cliver (~> 0.3.1)
websocket-driver (>= 0.2.0)
pry (0.12.2)
coderay (~> 1.1.0)
method_source (~> 0.9.0)
Expand Down Expand Up @@ -202,6 +198,7 @@ GEM
rspec-mocks (~> 3.9)
rspec-support (~> 3.9)
rspec-support (3.9.3)
rubyzip (2.3.0)
safe_yaml (1.0.5)
sassc (2.4.0)
ffi (~> 1.9)
Expand All @@ -212,6 +209,9 @@ GEM
sprockets-rails
tilt
selectize-rails (0.12.6)
selenium-webdriver (3.142.7)
childprocess (>= 0.5, < 4.0)
rubyzip (>= 1.2.2)
sentry-raven (3.0.0)
faraday (>= 1.0)
shoulda-matchers (4.3.0)
Expand Down Expand Up @@ -241,9 +241,6 @@ GEM
addressable (>= 2.3.6)
crack (>= 0.3.2)
hashdiff (>= 0.4.0, < 2.0.0)
websocket-driver (0.7.0)
websocket-extensions (>= 0.1.0)
websocket-extensions (0.1.5)
xpath (3.2.0)
nokogiri (~> 1.8)
zeitwerk (2.3.0)
Expand Down Expand Up @@ -271,12 +268,12 @@ DEPENDENCIES
i18n-tasks (= 0.9.31)
launchy
pg
poltergeist
pry-rails
pundit
rack-timeout
redcarpet
rspec-rails
selenium-webdriver
sentry-raven
shoulda-matchers
timecop
Expand Down
5 changes: 3 additions & 2 deletions spec/features/log_entries_index_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@
create(:log_entry)

visit admin_log_entries_path
click_on t("administrate.actions.destroy")

accept_confirm do
click_on t('administrate.actions.destroy')
nickcharlton marked this conversation as resolved.
Show resolved Hide resolved
end
expect(page).to have_flash(
t("administrate.controller.destroy.success", resource: "LogEntry"),
)
Expand Down
10 changes: 6 additions & 4 deletions spec/features/orders_index_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@
create(:order)

visit admin_orders_path
click_on t("administrate.actions.destroy")

accept_confirm do
click_on t('administrate.actions.destroy')
nickcharlton marked this conversation as resolved.
Show resolved Hide resolved
end
expect(page).to have_flash(
t("administrate.controller.destroy.success", resource: "Order")
)
Expand All @@ -60,8 +61,9 @@
create(:payment, order: create(:order))

visit admin_orders_path
click_on t("administrate.actions.destroy")

accept_confirm do
click_on t("administrate.actions.destroy")
nickcharlton marked this conversation as resolved.
Show resolved Hide resolved
end
expect(page).to have_flash(
"Cannot delete record because dependent payments exist", type: :error
)
Expand Down
22 changes: 15 additions & 7 deletions spec/rails_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

require "rspec/rails"
require "shoulda/matchers"
require "capybara/poltergeist"
require "selenium/webdriver"
nickcharlton marked this conversation as resolved.
Show resolved Hide resolved

Dir[Rails.root.join("../../spec/support/**/*.rb")].each { |file| require file }

Expand All @@ -31,14 +31,22 @@ module Features
end

ActiveRecord::Migration.maintain_test_schema!
Capybara.javascript_driver = :poltergeist
Capybara.register_driver :poltergeist do |app|
options = { phantomjs_options: ["--load-images=no"] }
Capybara::Poltergeist::Driver.new(app, options)

Capybara.register_driver :chrome do |app|
Capybara::Selenium::Driver.new(app, browser: :chrome)
end

Capybara.register_driver :rack_test do |app|
Capybara::RackTest::Driver.new(app, respect_data_method: false)
Capybara.register_driver :headless_chrome do |app|
capabilities = Selenium::WebDriver::Remote::Capabilities.chrome(
chromeOptions: {
args: %w[headless enable-features=NetworkService,NetworkServiceInProcess]
}
)
Capybara::Selenium::Driver.new app,
browser: :chrome,
desired_capabilities: capabilities
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this style of indent problematic. It breaks the indent for all lines if the first one is replaced with one of different length. How about writing it like this?

Suggested change
Capybara::Selenium::Driver.new app,
browser: :chrome,
desired_capabilities: capabilities
Capybara::Selenium::Driver.new(
app,
browser: :chrome,
desired_capabilities: capabilities,
)

To be honest, I'm not sure if we have a style rule for this, and Hound appears not to be enabled at the moment? @nickcharlton, do you have any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about the tmp/pids/.keep. I tried to fix the first clone problem you'd get in #1695, do we need to ignore that file too so if you delete it locally it doesn't matter?

Hound did run with this PR. It might've been done when the configuration in thoughtbot/guides was deleted. It got put back in again, so we might be alright now. A new push might highlight the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pablobm This indentation style was suggested by Houndbot, as was many of the changes you and Nick asked me to undo in this commit. I like yours better though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry about that. Normally we follow Hound's opinion to the letter, but a couple of weeks ago there was an issue with it. The set of rules changed and I'm not sure they are back to normal yet.

Don't worry then. Go with your preference and we'll come around to fixing Hound in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it’s been really annoying. I don’t much like commenting on style things manually, as even though consistency is good, nit-picking to the nth degree is not enjoyable for anyone!

I think there’s only one or two things left here: everything we can fix on GitHub apart from the tmp/pids/.keep file missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I've fixed all the issues now from the houndbot to the tmp/pids/.keep (which was in the gitignore so wasn't getting pushed). Thanks @nickcharlton

end

Capybara.javascript_driver = :selenium_chrome_headless

Capybara.server = :webrick
Empty file removed tmp/pids/.keep
View file
Open in desktop
Empty file.