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

Replace PhantomJS with Chromium #691

Merged
merged 5 commits into from
May 5, 2024
Merged

Replace PhantomJS with Chromium #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a28891a
Replacing abandoned PhantomJS with Chromium
mmx900 Apr 20, 2024
a9a3ff9
Fixing "Refuge is not defined" error
mmx900 Apr 20, 2024
85bf4c0
Removing unnecessary workaround
mmx900 Apr 20, 2024
133e1d7
Increasing timeout to prevent PendingConnectionError
mmx900 Apr 30, 2024
69cb0f3
Remove comments that are no longer needed
mmx900 May 1, 2024
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
16 changes: 1 addition & 15 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,10 @@ FROM ruby:3.2.2-slim

# Add basic binaries
RUN apt-get update \
&& apt-get install -y curl g++ gcc libfontconfig libpq-dev make patch xz-utils \
&& apt-get install -y curl g++ gcc libfontconfig libpq-dev make patch xz-utils chromium \
# Clean up the apt cache
&& rm -rf /var/lib/apt/lists/*

# Download, extract and install PhantomJS from archive hosted at bitbucket
RUN curl -L https://github.com/Medium/phantomjs/releases/download/v2.1.1/phantomjs-2.1.1-linux-x86_64.tar.bz2 -O \
# Check the file's integrity against its known sha1sum
&& test "`sha1sum phantomjs-2.1.1-linux-x86_64.tar.bz2`" = "f8afc8a24eec34c2badccc93812879a3d6f2caf3 phantomjs-2.1.1-linux-x86_64.tar.bz2" || (echo "PhantomJS tarball SHA1sum did not match!" && exit 1) \
# Extract and clean up the PhantomJS archive
&& tar xf phantomjs-2.1.1-linux-x86_64.tar.bz2 && rm phantomjs-2.1.1-linux-x86_64.tar.bz2 \
# Install PhantomJS binary to /usr/local/bin
&& mv phantomjs-2.1.1-linux-x86_64/bin/phantomjs /usr/local/bin \
# Clean up extra (un-needed) PhantomJS files
&& rm -rf phantomjs-2.1.1-linux-x86_64/

# Work around an issue with running "phantomjs --version"
ENV OPENSSL_CONF=/etc/ssl/

# Specify a major version of Node.js to download and install
ENV NODEJS_MAJOR_VERSION=16

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 @@ -54,7 +54,7 @@ end
group :test do
gem 'capybara'
gem 'database_cleaner'
gem 'poltergeist'
gem 'cuprite'
gem 'simplecov', '~> 0.22.0', require: false
gem 'webmock', '~> 3.12.1'
end
Expand Down
18 changes: 12 additions & 6 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ GEM
rack-test (>= 0.6.3)
regexp_parser (>= 1.5, < 3.0)
xpath (~> 3.2)
cliver (0.3.2)
coderay (1.1.3)
coffee-rails (4.2.2)
coffee-script (>= 2.2.0)
Expand All @@ -136,6 +135,9 @@ GEM
crack (0.4.5)
rexml
crass (1.0.6)
cuprite (0.15)
capybara (~> 3.0)
ferrum (~> 0.14.0)
database_cleaner (2.0.2)
database_cleaner-active_record (>= 2, < 3)
database_cleaner-active_record (2.1.0)
Expand Down Expand Up @@ -175,6 +177,11 @@ GEM
factory_bot_rails (4.8.2)
factory_bot (~> 4.8.2)
railties (>= 3.0.0)
ferrum (0.14)
addressable (~> 2.5)
concurrent-ruby (~> 1.1)
webrick (~> 1.7)
websocket-driver (>= 0.6, < 0.8)
ffi (1.16.3)
formtastic (5.0.0)
actionpack (>= 6.0.0)
Expand Down Expand Up @@ -270,6 +277,8 @@ GEM
net-smtp (0.4.0)
net-protocol
nio4r (2.7.0)
nokogiri (1.15.5-aarch64-linux)
racc (~> 1.4)
nokogiri (1.15.5-x86_64-darwin)
racc (~> 1.4)
nokogiri (1.15.5-x86_64-linux)
Expand All @@ -284,10 +293,6 @@ GEM
pg_search (2.3.6)
activerecord (>= 5.2)
activesupport (>= 5.2)
poltergeist (1.18.1)
capybara (>= 2.1, < 4)
cliver (~> 0.3.1)
websocket-driver (>= 0.2.0)
pry (0.14.2)
coderay (~> 1.1)
method_source (~> 1.0)
Expand Down Expand Up @@ -464,6 +469,7 @@ GEM
zeitwerk (2.6.12)

PLATFORMS
aarch64-linux
x86_64-darwin-19
x86_64-darwin-21
x86_64-linux
Expand All @@ -478,6 +484,7 @@ DEPENDENCIES
capybara
coffee-rails (~> 4.2)
country_select
cuprite
database_cleaner
devise (~> 4.8.1)
factory_bot_rails (~> 4.8.2)
Expand All @@ -497,7 +504,6 @@ DEPENDENCIES
pagy
pg
pg_search
poltergeist
pry
puma
rack-cors
Expand Down
27 changes: 6 additions & 21 deletions spec/support/locations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,15 @@ def locations
}
end

# rubocop:disable Metrics/MethodLength
def mock_location(location_name)
location = locations[location_name.to_sym]
page.execute_script "
var __bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; };

Refuge.Library.Geocoder = (function() {
function Geocoder() {
this.getAddress = __bind(this.getAddress, this);
this.geocodeSearchString = __bind(this.geocodeSearchString, this);
}

Geocoder.prototype.getCurrentLocation = function() {
return jQuery.when(
{
latitude: #{location[:latitude]},
longitude: #{location[:longitude]}
});
};

return Geocoder;

})();
navigator.geolocation.getCurrentPosition = function(success, failure) {
success({ coords: {
latitude: #{location[:latitude]},
longitude: #{location[:longitude]}
}, timestamp: Date.now() });
}
Comment on lines -14 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is mostly just moving to modern JS promises instead of JQuery stuff. Which is good, I suppose, perhaps not using standard promises was why this spec broke? Hmm.

Anyhow, it's working, which is the most important thing!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually a little bit unclear why we were defining this as Refuge.Library.Geocoder.getCurrentLocation() before, and it's being defined as navigator.geolocation.getCurrentPosition() now.

But, unfortunately, I'm just not very versed in the underlying code of this web app, I've been focused on bugfixes and dependency updates. So, if this works, I'm inclined to believe it is the right thing to do.

Would like to understand better why it worked as Refuge.Library.Geocoder.getCurrentLocation() before, stopped working, and works as navigator.geolocation.getCurrentPosition() now. But I'm not sure if it's worth it to hold up the PR until this can be explained better, and my not understanding it may be down to my own unfamiliarity with the code to begin with.

I can mainly say the new code looks cleaner and more readable, that's about it. To be quite transparent.

Thank you anyway! Regardless of if you wish to try to explain this or not, the fix is appreciated either way!

Copy link
Contributor Author

@mmx900 mmx900 May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

At the time that the code was written(69d417), Geocoder was Refuge.Library.Geocoder(69d417). Then It was rewritten to Geocoder, applying ES6/Webpacker. However, this change did not cause any test failure for two reasons :

  1. Poltergeist was set to ignore javascript errors with js_errors: false. So you could replace the code with anything like asdf.qwer.blah() without causing any test failure.
  2. The tests using mock_location in restroom_spec.rb seems never to have been written completely since cd52015 except for one test that didn't require an exact location. Some expectation codes are still commented out, and some specs are just wrapped with #rubocop:disable RSpec/NoExpectationExample. So I think that the return values of getCurrentLocation() would never be a problem. I'll try to complete them later.

Thanks for review this PR.

"
end
# rubocop:enable Metrics/MethodLength
end
12 changes: 4 additions & 8 deletions spec/support/rspec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require 'capybara/poltergeist'
require 'capybara/cuprite'
require 'capybara/rspec'
require 'rspec/rails'
require 'json'
Expand All @@ -7,15 +7,11 @@

require_relative './locations'

Capybara.register_driver :poltergeist_debug do |app|
Capybara::Poltergeist::Driver.new(
app,
js_errors: false
)
Capybara.javascript_driver = :cuprite
Capybara.register_driver(:cuprite) do |app|
Capybara::Cuprite::Driver.new(app, window_size: [1200, 800], browser_options: { 'no-sandbox': nil }, timeout: 30)
end

Capybara.javascript_driver = :poltergeist_debug

WebMock.disable_net_connect!(allow_localhost: true)

RSpec.configure do |config|
Expand Down