-
Notifications
You must be signed in to change notification settings - Fork 983
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.
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
fixes #21592 - uses headless chrome instead of phantomjs #4987
Conversation
Issues: #21592 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change, approved pending green tests.
@mmoll @ekohl any idea why the test fail? I'm installing chromedriver via npm (search from chromedriver in http://ci.theforeman.org/job/test_develop_pr_core/14979/database=postgresql,ruby=2.2,slave=fast/consoleFull) and I'm using the relative path at https://github.com/theforeman/foreman/pull/4987/files#diff-7a6aaeae51145fd37e71db58212fd18bR15) which of course works locally. how can I debug / troubleshoot this? |
According to https://github.com/SeleniumHQ/selenium/wiki/ChromeDriver it expects chrome to be in |
I was under the assumption that I was override it - (its possible via
https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-Using-a-Chrome-executable-in-a-non-standard-location
)
also, we could consider setting up a symlink?
…On Wed, Nov 8, 2017 at 11:03 AM, Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
According to https://github.com/SeleniumHQ/selenium/wiki/ChromeDriver it
expects chrome to be in /usr/bin/google-chrome. You're just installing
the driver and I think we need to look at a package installation of chrome.
A quick search for RPMs doesn't show me any for centos7 so I'll need to
dive into it. Since it's selenium it should also support firefox which may
work better on centos7 slaves. Need to dive into it a bit to figure out our
options.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4987 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABOx1g61pQOp2iOo0XfK45gy3n8OHBIks5s0W5JgaJpZM4QUtDX>
.
|
But the entire chrome browser isn't on NPM, only the chrome driver for selenium. One reason I can think of why chrome isn't packaged for EL7 is that it uses some kernel features for sandboxing that the EL7 kernel doesn't support, but this is speculation from my side. It may be that the Fedora packages do work. One path we can choose is to get Fedora slaves for the UI tests but I'd prefer to try a more generic solution first with Firefox. |
On Wed, Nov 8, 2017 at 11:08 AM, Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
But the entire chrome browser isn't on NPM, only the chrome driver for
selenium. One reason I can think of why chrome isn't packaged for EL7 is
that it uses some kernel features for sandboxing that the EL7 kernel
doesn't support, but this is speculation from my side. It may be that the
Fedora packages do work. One path we can choose is to get Fedora slaves for
the UI tests but I'd prefer to try a more generic solution first with
Firefox.
Are you suggesting we will drop this pr in favor of firefox based
approach?
|
That would be my suggestion to start with. I have experience with using Firefox + Selenium on CentOS7 and generally that works well. If the setup code is generic enough we can support chrome via an env var for users who prefer that on their desktops. Note that Firefox and xvfb are already installed on our slaves but the job config may need to be modified. Looks like the 1.11-stable job uses it. |
On Wed, Nov 8, 2017 at 11:14 AM, Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
That would be my suggestion to start with. I have experience with using
Firefox + Selenium on CentOS7 and generally that works well. If the setup
code is generic enough we can support chrome via an env var for users who
prefer that on their desktops.
Note that Firefox and xvfb are already installed on our slaves but the job
config may need to be modified. Looks like the 1.11-stable job uses it.
the fact is that currently the tests are: a. slow b. inaccurate as they
represent an old browser env (which imho is no longer relevant to our users
- e.g. IE). I would like to unblock / improve our test infrastructure.
whats the timeline you had in mind to implement this? thanks.
|
That makes total sense, but I haven't thought about it. We have a few priorities to balance no. I think that Rails 5.1 SCL + Updated Ruby is also a high priority blocker for developers but the fixing the Dynflow service for the 1.16 release is my top priority. Perhaps this is small enough to do it before the Rails 5.1 effort but I'll get back to you on that. |
another option is to run it on travis - as its already configured to support chrome. |
Travis has matrix builds. See https://github.com/theforeman/puppet-foreman/blob/master/.travis.yml for how we use separate RVMs. Pretty sure you can choose a different language in the |
I failed to make this work (tried at
ManageIQ/manageiq-ui-classic#2540) without success.
…On Wed, Nov 8, 2017 at 11:33 AM, Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
Travis has matrix builds. See https://github.com/theforeman/
puppet-foreman/blob/master/.travis.yml for how we use separate RVMs.
Pretty sure you can choose a different language in the include.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4987 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABOx7RcA5atzFRv5HiQOkPqEqilqbZMks5s0XVRgaJpZM4QUtDX>
.
|
It appears you're right: travis-ci/travis-ci#4090 |
The Travis configuration is already running node based tests. Where does the Ruby part come into play? |
@ehelms chrome integration tests run against the rails server.... you need to be able to execute |
One broader issue I will point out is that our current standard devel environment via Forklift is based on using CentOS 7 given that is the production RPM distro we support. This change would imply that developers could no longer run these tests within the local environment. Has anyone tried installing the 'Fedora' only Chrome on EL7? From what I read, when they dropped it they dropped it intending to drop for EL6 but dropped it for all. Should be a fairly easy test on a Forklift environment to configure:
Install chrome and try this PR out. If that works, then we can add this to our slaves no? |
@ohadlevy ahh so these are for the older JS tests not the new ones coming with the React changes? Or even those requires a running Rails server? |
this PR aim is to replace phantomjs with headless chrome- the motivation is to:
regarding the tests itself:
|
So testing this out, I was able to install Chrome on EL7 without issues (running the tests now) using that repository. From what I read, they "pulled" Chrome from EL6 due to incompatibility and in the process pulled it from all all EL despite it still working just fine. Assuming tests pass, I'd opt for adding this repository to our slaves and allowing the use of Chrome here. |
@ehelms I think that's fine. |
@ehelms whats the next step ? |
In order to use chrome headless with Selenium, and get rid of phantomjs. For the moment, both can be installed to test theforeman/foreman#4987 When that PR is merged and we no longer need phantomjs and firefox, we can remove them on another PR.
In order to use chrome headless with Selenium, and get rid of phantomjs. For the moment, both can be installed to test theforeman/foreman#4987 When that PR is merged and we no longer need phantomjs and firefox, we can remove them on another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ohadlevy @ehelms @ekohl I think a good strategy to move forward is:
- Installing chrome alongside firefox/phantomjs in our slaves
- Making sure tests pass on Jenkins in this PR
- Remove phantomjs/firefox
I've submitted theforeman/foreman-infra#381, as soon as that is merged please trigger the tests here again to see if they pass. I'd be thrilled to see a default_max_wait_time=5
working 😄
In order to use chrome headless with Selenium, and get rid of phantomjs. For the moment, both can be installed to test theforeman/foreman#4987 When that PR is merged and we no longer need phantomjs and firefox, we can remove them on another PR.
In order to use chrome headless with Selenium, and get rid of phantomjs. For the moment, both can be installed to test theforeman/foreman#4987 When that PR is merged and we no longer need phantomjs and firefox, we can remove them on another PR.
@ohadlevy a lot of tests fail and I have the feeling a lot of tests are run with this new configuration the first time after a long while... how can that be? |
[test foreman] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ohadlevy I think this needs rebasing so tests pass, could you try that? Thanks!
bb61011
to
9ee880e
Compare
rebased, no change. |
9ee880e
to
c145a62
Compare
c145a62
to
cef1974
Compare
Currently not, as it was agreed to use the chromium version coming in EPEL. |
@@ -213,6 +212,7 @@ def database_cleaner_strategy | |||
|
|||
def login_admin | |||
SSO.register_method(TestSSO) | |||
visit '/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this fixes some tests, some are also failing because of this (e.g. TEST=test/integration/fact_value_test.rb
)
I think the changes itself are OK, somebody:tm: would need to have a look into the problem with the cookies vs. visting a page first and then fix a few remaining problems. |
it seems that we can also cut down the default waiting time which hopefully will speed up our tests.
closing in favor of #5770 |
it seems that we can also cut down the default waiting time
which hopefully will speed up our tests.