Replace PhantomJS with Chromium #691
Conversation
|
Thank you for this pull request. I'll take a look at it. |
Fair question. I would guess something changed in one of our dependencies, perhaps the Google Maps library. I ran a passing CI run of 2c70ab9 back on the 10th of May 2023. (Passing run: https://app.travis-ci.com/github/RefugeRestrooms/refugerestrooms/builds/262958108). And then did a CI run with no code changes, to see if things were still in good shape, on the 4th of October 2023, which failed. (Failing run with no code changes 5 months later: https://app.travis-ci.com/github/RefugeRestrooms/refugerestrooms/builds/266379502). (This revealed another, separate issue with the old Travis-CI Xenial CI environment, which was fixed by updating Travis-CI to use a newer base OS. Fixed by: #683. But we still have the one failing spec, in our out of CI, even after fixing the Ubuntu Xenial CI issue.) So, I suspect one or more of our many dependencies changed "out from under us," so to speak. We can't pin to a specific Google Maps library version anymore, only define the pace of updates as "weekly" or "quarterly", if I recall correctly. Since most of our other dependencies are locked in our For reference, you can see our total CI build history here: https://app.travis-ci.com/github/RefugeRestrooms/refugerestrooms/builds. |
|
Sorry for the delay, I've been working on getting CI running again for this repo, plus attending to some IRL things. There is a A fuller snippet of the log output: Those are the logs I gathered from getting this PR running in GitHub Actions: https://github.com/DeeDeeG/refugerestrooms/actions/runs/8868660490/job/24348476133 Similar error in Travis CI: https://app.travis-ci.com/github/DeeDeeG/refugerestrooms/builds/270094473#L2761 |
|
Thanks for the detailed explanation. I've incresed the |
| 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() }); | ||
| } |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 :
- Poltergeist was set to ignore javascript errors with
js_errors: false. So you could replace the code with anything likeasdf.qwer.blah()without causing any test failure. - The tests using
mock_locationinrestroom_spec.rbseems 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 ofgetCurrentLocation()would never be a problem. I'll try to complete them later.
Thanks for review this PR.
* Replacing abandoned PhantomJS with Chromium * Fixing "Refuge is not defined" error * Removing unnecessary workaround * Increasing timeout to prevent PendingConnectionError * Remove comments that are no longer needed
Context
Digging the test failure of develop branch, I found that the project is still using old PhantomJS(with Poltergeist) which is outedated and doesn't support ES6 features. When I try to debug using
page.driver.debug, I can see console errors includingUnexpected token 'const'. As I remember there are many weird bugs in PhantomJS. So I just replaced that with Chromium/Cuprite/Ferrum. Now all tests are passed in my local environment withdocker compose run -e "RAILS_ENV=test" web rake db:test:prepare spec. But I'm not sure why it was OK before 2c70ab9.Summary of Changes
Refuge is not definederror.Checklist