WIP: Replace PhantomJS with headless Chrome for Rspec and Spinach tests

Ok, I've updated the image to the latest version of Chrome and ChromeDriver to 2.31, but as you can see there are lots of test failures now unrelated to xvfb now. :)

marked the checklist item unknown error: an X display is required for keycode conversions, consider using Xvfb (e.g. https://gitlab.com/gitlab-org/gitlab-ce/builds/18894684). as completed

added 1 commit

fa68b81b - Change trigger('click') -> click for Chrome

Compare with previous version

added 1 commit

ded77e21 - Rename remove_cookie -> delete_cookie in Capybara helper for Selenium driver

Compare with previous version

changed the description

@stanhu Thanks for putting the actual command line you used to do this mass-replace! :)

mentioned in merge request !12931 (merged)

Selenium/Webdriver API does not support status_code and this issue has been labeled wontfix by its maintainers. Our best option would be to adopt some sort of HTTP library to test status codes instead of capybara for the tests that rely on them.

added 982 commits

ded77e21...2925850c - 981 commits from branch master
09baadca - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

assigned to @mikegreiling

added 1 commit

50701b58 - Fix implementation of resize_window for Selenium

Compare with previous version

added 1 commit

4b548332 - Change trigger('focus') -> click

Compare with previous version

changed the description

added 1 commit

a5e096ce - Expand Chrome window size to make specs pass

Compare with previous version

added 1 commit

08172ff5 - Expand Spinach window size and remove unsupported trigger calls

Compare with previous version

added 1 commit

7b70d012 - Add return value in form input to make add member specs pass

Compare with previous version

added 124 commits

7b70d012...2b950014 - 123 commits from branch master
07f8105f - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

While watching TV, I got a bunch more specs to pass here. :) There are a number of unsupported features in Selenium (e.g. retrieving HTTP status codes/headers, inspecting network calls, etc.) that are being used in GitLab with PhantomJS. I think we're better off removing them and/or adding those checks into the controller, if possible.

I also found two bugs in Capybara when trying to call accept_alert in headless Chrome:

UPDATE: These changes aren't needed if we update the seleinum-webdriver gem, which I have already done.

added 2 commits

0d6133b9 - Bump Capybara version for Chrome headless fixes
2901fc13 - Make protected branches spec work in headless Chrome

Compare with previous version

added 1 commit

ad73d242 - Bump selenium-webdriver to 3.5.0 to make Capybara work properly

Compare with previous version

added 2 commits

1eade585 - Fix trigger spec by surrounding the accept_confirm in the right place
f64f06f6 - Change trigger('click') calls to click

Compare with previous version

added 3 commits

1ddfe8ca - Remove a few HTTP status checks as these are not supported in Selenium
b03fe23d - Remove unsupported network_traffic in specs
12e6cb71 - Fix pipelines spec by surrounding click with accept_confirm

Compare with previous version

awesome! I’m going to try to devote some time to this next week in between my other deliverables.

added 1 commit

93077d76 - Fix merge request Spinach test for Selenium

Compare with previous version

Doing some further research into methods we can use to obtain HTTP header and status code information from the requests in our integration tests...

In the lengthy selenium issue discussion about why they don't intend to support this feature, they suggest using something like BrowserMob Proxy to obtain these data.

Having looked into BrowserMob proxy, there are some serious downsides... for one, it requires a JAVA runtime which we'd have to bake into our build images. It also apparently has some irritating bugs with the ruby bindings available.

This article lays out some alternatives through the use of browser extensions, and it sounds like a much more workable solution to me.

added 1 commit

a7c1ce3f - Fix Spinach spec for deleting branches

Compare with previous version

added 1 commit

a2cd32b7 - Fix snippets spec: send_keys only works with text areas

Compare with previous version

added 1 commit

247138d4 - Accept the confirm modal to make comment on commit spec to pass

Compare with previous version

So, here's the idea I'm running with at the moment...

We can create a really simple rack middleware that is only injected on Rails.env.test? (similar to how we implement RequestBlockerMiddleware). This middleware will log the most recent request and response info to emulate page.status_code, page.response_headers, and page.network_traffic for inspection via some rspec helper methods (something like last_request.status_code or last_request.response_headers).

added 1 commit

3e644d4b - Fix U2F spec in headless Chrome

Compare with previous version

added 1 commit

fab88bc3 - Fix a few failing tests in Spinach merge request specs for Selenium

Compare with previous version

added 1 commit

767dffcc - Fix a few failing specs due to missing accept_confirm

Compare with previous version

added 202 commits

767dffcc...441ba5f3 - 200 commits from branch master
d3e81673 - Merge branch 'master' into sh-headless-chrome-support
7b5722c8 - Fix a number of failed Spinach tests with Selenium needing accept_alert

Compare with previous version

added 1 commit

ebd5c309 - Add missing Capybara select2 helper

Compare with previous version

added 61 commits

ebd5c309...36ba84cb - 59 commits from branch master
541a082c - Rename find('.ace_editor') -> find('.ace_text-input') to make Chrome happy
3e75b7fa - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 61 commits

ebd5c309...36ba84cb - 59 commits from branch master
541a082c - Rename find('.ace_editor') -> find('.ace_text-input') to make Chrome happy
3e75b7fa - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

changed the description

added 91 commits

3e75b7fa...84336b84 - 90 commits from branch master
841a5ef5 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 91 commits

3e75b7fa...84336b84 - 90 commits from branch master
841a5ef5 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 156 commits

841a5ef5...ce578272 - 154 commits from branch master
7b223f38 - Merge branch 'master' into sh-headless-chrome-support
4a4aae7f - Bump .gitlab-ci image to use git 2.13

Compare with previous version

added 1 commit

dda33bd7 - Rename JavaScript click -> click() to make karma tests pass

Compare with previous version

added 1 commit

7b79ec07 - Fix milestone Spinach tests by confirming modal only if it is present

Compare with previous version

added 1 commit

3e4b7e98 - Use find_link instead of incorrect jQuery to make Selenium test pass

Compare with previous version

@mikegreiling I wonder if https://github.com/miyagawa/rack-vcr would work to capture the network calls.

changed the description

added 100 commits

3e4b7e98...19dfd9e9 - 97 commits from branch master
127a986d - Merge branch 'master' into sh-headless-chrome-support
55e5c84e - Merge branch 'master' into sh-headless-chrome-support
4ab7d2f9 - Fix branch creation Spinach test by clicking on the dropdown

Compare with previous version

added 1 commit

1899d543 - Change trigger('click') and trigger('mouseover') -> click

Compare with previous version

added 1 commit

07e95212 - Fix recent_searches_spec.rb by clicking on buttons instead of relying on hidden elements

Compare with previous version

mentioned in issue #36961

Tried to get this working locally but not really sure how to install chromedriver. Will have to do more research. Will post detail instructions later

Was going to help with this but @jschatz1 wanted me to work on repo editor first

added 1 commit

f2c60eba - Fix create branch Spinach spec

Compare with previous version

added 704 commits

f0459035...685066cd - 703 commits from branch master
41e5ec8f - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 1 commit

2d58626a - Changed helper methods to use the click method

Compare with previous version

I've worked on this recently, is it okay for me to merge master into this? it could help with testing some things locally

@jivanvl Yes, I've been merging master here. Please feel free to do that, but do not rebase.

@stanhu I tried to merge master into this, but some of the specs that were already passing started to fail locally. Is there any chance that we could pair on the conflict resolution? Some of those are really tricky

added 660 commits

2d58626a...33010da2 - 659 commits from branch master
4c0beb6c - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

Since this is a very large set of changes, and some of these changes are likely backward-compatible with our PhantomJS/Poltergeist powered specs, I wonder if it would be a good idea to start back-porting as much of these test fixes as possible into master as we do them. This way our ultimate headless Chrome MR will be leaner and less prone to merge conflicts as we continuously merge changes from master.

When I was working on converting our frontend specs from Teaspoon to Karma, I took this same approach. In the end, all of the tests were able to pass both environments and then the final step was to just make the switch.

We might have a showstopper with tests that require cookies, take a look at spec/features/merge_requests/user_posts_diff_notes_spec.rb

It requires to set a cookie prior to each test, so if I try to use the set_cookie method from capybara we get a:

undefined method `set_cookie' for #<Capybara::Selenium::Driver:0x00000005ac0580>

So ok this could be worked around using the execute_script method from the selenium web driver and set the cookie from the document itself. So if I do:

page.driver.execute_script("document.cookie='galleta=true';")

I get a

Failed to set the 'cookie' property on 'Document': Access is denied for this document

So I found it weird that there's no support to adding cookies via the chrome driver and according to the spec we have a "partially complete" support on the "Add Cookie" feature.

https://chromium.googlesource.com/chromium/src/+/master/docs/chromedriver_status.md

We might have to wait a bit until to fully complete the migration of our test suite to chrome headless until this is solved

added 166 commits

4c0beb6c...9b137533 - 165 commits from branch master
27a28d99 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

merged master again. also updated the build image to the latest one with Chrome 61.0

resolved all discussions

We might have a showstopper with tests that require cookies, take a look at spec/features/merge_requests/user_posts_diff_notes_spec.rb

It requires to set a cookie prior to each test, so if I try to use the set_cookie method from capybara we get a:
undefined method `set_cookie' for #<Capybara::Selenium::Driver:0x00000005ac0580>

I don't think this is a problem. After spending some time researching this, it seems we can set cookies with selenium by using page.driver.browser.manage.add_cookie.

Here's how we need to modify the syntax:

# before
page.driver.set_cookie('sidebar_collapsed', 'true')

# after
visit '/'
page.driver.browser.manage.add_cookie(name: 'sidebar_collapsed', value: 'true')

The visit '/' is important because add_cookie will fail when the browser is currently at an initial about:blank page. You need to visit some page in the app's domain before you can set cookies...

We could create a simple helper method for this.

The visit '/' is important because add_cookie will fail when the browser is currently at an initial about:blank page. You need to visit some page in the app's domain before you can set cookies...

@mikegreiling In which case, we should probably visit the 404 page, as its static and thus way faster. Maybe we should wrap this with a method which that can be called in a before block, that registers the unset action by itself.

@zj unfortunately using the / root path is the only one I’ve managed to get working. It seems that the cookie path gets set to whatever the current page’s path is, meaning that cookie will only work in pages who’s path includes the cookie’s path (passing path: '/some/path' to set this explicitly doesn’t seem to work).

If the path were /404.html I don’t know if the cookie would work in /foo/bar/merge_requests/12/diffs

Has anyone been able to handle header manipulation with Selenium? I've been looking around and well things don't look that promising.

Apparently Selenium WebDriver API doesn't support this, according to this GitHub issue

There are some recommended workarounds floating around, but I was wondering if anyone else has done some research of this topic beforehand.

There was some discussion about this earlier in the thread: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12244#note_37466296

Good to hear that you already though of this, thanks Mike!

added 1 commit

d7b48eb0 - Replaced trigger calls for click

Compare with previous version

added 1 commit

d4f3fca9 - fix broken add_cookies

Compare with previous version

added 198 commits

d4f3fca9...137c5822 - 197 commits from branch master
76de6327 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 1 commit

7a8514c3 - add set_cookie helper

Compare with previous version

I've added a set_cookie helper that should perform the necessary steps to apply cookies in Selenium/WebDriver. I've also updated all of the tests I could find which were using set_cookie.

Hopefully this takes care of that issue.

added 1 commit

bea18efa - Replaced trigger calls for click

Compare with previous version

added 1 commit

299213dd - Replaced trigger calls for click, fixed click_on call

Compare with previous version

added 1 commit

7fb47732 - fixed users_spec

Compare with previous version

added 165 commits

7fb47732...3b2d68d3 - 164 commits from branch master
b4af5468 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 3 commits

c01338cb - fixed spec/features/u2f_spec.rb
54a6bcae - fixed spec/features/projects/files/edit_file_soft_wrap_spec.rb
d770b633 - fixed spec/features/projects/settings/repository_settings_spec.rb

Compare with previous version

added 5 commits

92a3887c - fixed spec/features/issues/filtered_search/dropdown_assignee_spec.rb
9d48177c - fixed spec/features/admin/admin_users_spec.rb
7a090d59 - fixed spec/features/admin/admin_groups_spec.rb
27c00d2e - fixed spec/features/dashboard/groups_list_spec.rb
1ec2486c - fixed spec/features/projects/deploy_keys_spec.rb

Compare with previous version

added 4 commits

15b976c6 - some fixes in spec/features/projects/jobs_spec.rb
4a613362 - fixed spec/features/issues/filtered_search/dropdown_milestone_spec.rb
d2561680 - fixed spec/features/merge_requests/versions_spec.rb
45da02f9 - fixed spec/features/merge_requests/user_posts_notes_spec.rb

Compare with previous version

added 1 commit

5072ecc6 - Replaced trigger click calls for click, also fixed ace_editor-input test

Compare with previous version

added 2 commits

06d41721 - modified clear_browser_session method to use the manage method for deleting cookies
b452b0c7 - Used send_keys(:return) for elements that are not clickable at one point

Compare with previous version

added 209 commits

b452b0c7...a7976905 - 208 commits from branch master
bd2822ce - Merge remote-tracking branch 'origin/master' into sh-headless-chrome-support

Compare with previous version

added 1 commit

7a5c2d5e - Fix Element is not clickable at this point

Compare with previous version

added 6 commits

bea4b0a8 - fix admin_hooks_spec
ac417cb3 - fix admin_uses_repository_checks
ebddd8d4 - fix diff_notes_spec
6c541eda - fix spec/features/projects/import_export/export_file_spec.rb
c3d03582 - fix export_file_spec
62364576 - fix mattermost_slash_commands

Compare with previous version

added 6 commits

bea4b0a8 - fix admin_hooks_spec
ac417cb3 - fix admin_uses_repository_checks
ebddd8d4 - fix diff_notes_spec
6c541eda - fix spec/features/projects/import_export/export_file_spec.rb
c3d03582 - fix export_file_spec
62364576 - fix mattermost_slash_commands

Compare with previous version

added 166 commits

62364576...18fee306 - 165 commits from branch master
f682cc18 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

Looks like the ChromeDriver has been updated to 2.32

https://sites.google.com/a/chromium.org/chromedriver/downloads

mentioned in merge request gitlab-build-images!59 (merged)

@jivanvl I was incorrect earlier when I said we hadn't locked our chromedriver version. I have opened a MR in gitlab-build-images!59 (merged) to update it.

added 36 commits

f682cc18...5a23af92 - 35 commits from branch master
356e9aa8 - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

added 3 commits

5abcedf4 - fix rubocop
10c70b19 - fix admin_settings_spec
7449c45d - fix add_issues_modal_spec

Compare with previous version

When I was trying this particular spec gfm_autocomplete_spec.rb I found out that the ChromeDriver does not support unicode characters from the SMP plane and beyond, only from the BMP plane. So when you try to test something like this

'💃speciąl someone💃'

Chrome will throw this particular error:

Selenium::WebDriver::Error::UnknownError:
       unknown error: ChromeDriver only supports characters in the BMP

I found out that the team behind the chrome driver did manage to get a fix in but the last comment of this thread points out to not being fixed

https://bugs.chromium.org/p/chromedriver/issues/detail?id=187#c12

Any ideas on a work around for this?

added 1 commit

035bf5d4 - fix create_snipper and diff_notes specs

Compare with previous version

added 466 commits

035bf5d4...3cbab382 - 465 commits from branch master
02838d5b - Merge branch 'master' into sh-headless-chrome-support

Compare with previous version

mentioned in commit simonwex/gitlab-ce@5d4f377b

Admin message

Admin message

WIP: Replace PhantomJS with headless Chrome for Rspec and Spinach tests

Merge request reports

Activity