Ew replace phantomjs with selenium 2 #1718
Conversation
…tomjs has been discontinued. Three tests currently failing due to new webdriver
Instead of `click_on t()` now require `accept_confirm do click_on t() end`
There was a problem hiding this comment.
Sorry, I realised on second view that there were a couple of minor issues. Mind having a look?
Also, the file tmp/pids/.keep has been removed. No surprise as there's something going on where it's deleted every time the server runs. I have noticed it before and I don't know what's going on. @nickcharlton, any ideas as to why?
For now, if you could just reinstate it with touch tmp/pids/.keep, that would be great.
spec/rails_helper.rb
Outdated
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| require "rspec/rails" | |||
| require "shoulda/matchers" | |||
| require "capybara/poltergeist" | |||
| require 'selenium/webdriver' | |||
There was a problem hiding this comment.
Actually, I missed this. Mind changing the single quotes to double, for consistency?
spec/rails_helper.rb
Outdated
| Capybara::Selenium::Driver.new app, | ||
| browser: :chrome, | ||
| desired_capabilities: capabilities |
There was a problem hiding this comment.
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?
| 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Co-authored-by: Nick Charlton <nick@nickcharlton.net>
Co-authored-by: Nick Charlton <nick@nickcharlton.net>
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
|
Mind just adding back that |
Replaced phantomjs with selenium in gemfile and rails_helper, as phantomjs has been discontinued. Three tests currently failing due to new webdriver: ``` click_on t() ``` …which are fixed with: ``` accept_confirm do click_on t() end ```
When #1718 was merged, it introduced a potential solution to /tmp/pids/.keep being unintentionally deleted. Unfortunately this didn't fix the issue and in fact seems to have made it worse. The issue may have been solved already, and it was only surfacing again when moving between commits that included #a0eeca6 and those that didn't. The changes I'm proposing to gitignore are as currently generated in new Rails apps.
Updated the discontinued phantomjs with selenuim webdirvers and fixed related tests.
Update of pull request to be based off master branch instead of unmerged pull requests.