Problem/Motivation
We are using phantomjs for testing our javascript behaviour.
For the communication with phantomjs we are leveraging gastonjs, which is a port of poltergeist, a quite common library in the ruby and java world.
Poltergeist uses direct communication with phantomjs to control the browser.
There are various problems with this:
- GastonJS is not that much supported and doesn't have a huge userbase.
- We are using phantomjs, which is not necessarily maintained and stable, especially compared to a headless chrome
An alternative approach is to leverage webdriver, a W3C API, which allows you to
control both phantomjs as well as actual browsers like firefox/chrome using selenium.
This would have the following advantages:
- You can easier see what is happening in a real browser
- It is more common than gastonJS to use
Proposed resolution
Open an issue in the testbot issue queue to still use phantomjs, but using the--webdriver
command #2857788: Patch the testbot to start both chrome in webdriver mode and phantomjs in non webdriver mode- Use the mink selenium driver, instead of the gastonjs one. (this issue)
- More details below
- Profit?
Remaining tasks
User interface changes
API changes
Data model changes
Testing if Drupal can run with webDriver [1] which allowed running tests under real Browsers.[2]
[1] Bahat official webDriver: http://mink.behat.org/en/latest/drivers/selenium2.html
[2] Testing with different browsers: http://www.seleniumhq.org/download/
Pre-setup:
1. Apply patch
Testing (chrome):
- brew install selenium-server-standalone
- brew install chromedriver
- selenium-server -port 4444
- export MINK_DRIVER_ARGS_WEBDRIVER='["chrome", null, "http://localhost:4444/wd/hub"]'
- sudo -u _www ./vendor/bin/phpunit -v -c ./core core/tests/Drupal/FunctionalJavascriptTests/Tests/JSWebWithWebDriverAssertTest.php
Comment | File | Size | Author |
---|---|---|---|
#133 | 2775653-133.patch | 13.77 KB | alexpott |
#133 | 132-133-interdiff.txt | 2.37 KB | alexpott |
#132 | 2775653-132.patch | 12.57 KB | alexpott |
#132 | 124-132-interdiff.txt | 1.94 KB | alexpott |
#124 | 2775653-124.patch | 13.3 KB | Lendude |
Comments
Comment #2
droplet CreditAttribution: droplet commentedComment #3
dawehnerOf course it works, this is the entire point of not using phantomJS directly, but rather a browser abstraction layer (mink).
Personally I just don't want to deal with fighting the testbots, they aren't really a moving target, nor are they particularly easy to deal wth
Comment #4
dawehnerI don't want to sound bitter, but I think the time would be better invested in extending our test coverage in the javascript space first.
Comment #5
droplet CreditAttribution: droplet commentedIt would be more interesting after seeing the comments. Wasn't Selenium2Driver available at that point? I'm surprised how easy to run it with Drupal is.
With webDriver, it's easier to inspect via browser dev tools instead of many screenshots to guess how the `document` is. We will know how the test engine performs tests, not how we using Mouse & KB to simulate it when you write test cases..etc. For example in this case #2717629: Add filter group for a View fails. @Lendude told that my test case was wrong. But looking at my code, it's pretty straightforward and no errors. With browsers, we can see the visual result of each step in real-time. Dropping a breakpoint and inspect it and move to next point. It speeds up to write ( & review) testcase actually.
In another side, we will get better supports from active Behat & PHP WebDriver (Facebook) communities.
Comment #6
dawehnerOne reason we went with phantomjs: We wanted to check for HTTP status codes. This is not exposed by the selenium API but just by phantomjs directly.
As written before, the other reason is that getting a real browser onto the testbots would be a major effort.
But sure, feel free to spend the time and trying it out :) It would be great to use selenium actually, because selenium provides a selenium grid we need at
point to scale up the javascript tests.
Comment #7
dawehnerSo yeah please open up an issue in the testbot queue. I would like to see what happens :)
Comment #9
dawehner@droplet
If you want to make this change, can you open up the corresponding testbot issue?
Comment #10
chx CreditAttribution: chx at Smartsheet, Migrate Rocks commentedI tried chromedriver and I can report this works to an extent.
I believe the click method might be slightly broken with chromedriver because
#divFormBody > div.clsJspFormBody > div > table > tbody > tr:nth-child(3) > td > table:nth-child(1) > tbody > tr > td > a
doesn't work but using//*[@id="divFormBody"]/div[2]/div/table/tbody/tr[3]/td/table[1]/tbody/tr/td/a
does. It might be broken with more than chromedriver but my time debugging this is up. A separate issue might be in order.Comment #11
dawehnerHere is a more constructive issue summary
Comment #12
dawehnerI adapted the patch to be a bit more minimal, like removing gastonjs, adding selenium etc. and also adapting the documentation.
Comment #13
droplet CreditAttribution: droplet commentedFixed:
- Fixing set Cookie in PhantomJS
- Mute StatusCode in JSTests @see comments in interdiff.patch
Known issues:
- StatusCode needs further discussion.
- 2 or 3 tests failing. @see: Attached logs.txt
Notes:
- All errors are fixable from my reviews. Those should be improved to current testbots system also. I will send Patches to each Component issues threads soon.
Comment #15
droplet CreditAttribution: droplet commentedSame as #13. (Fixing the new file index)
Comment #16
dawehnerNote: The missing feature to be able to retrieve the HTTP status code was one of the reasons to use gastonjs instead of selenium. The same is true for HTTP headers
Isn't there a way to execute some JS to detect that information? IMHO this is something we should try to discuss, can we live with that missing feature, before we invest too much work.
Comment #17
droplet CreditAttribution: droplet commentedDo you still remember the issue for "HTTP status code" discussion? Would like to understand the reason & background info. Thanks.
Is it because we use it as UnitTests (in future)?
Impossible since it isn't requested from JS. PhantomJS HTTP Status provided by PhantomJS API. Selenium rejected to implement the HTTP Status API in WebDriver. Use Proxy is an option..
Comment #18
dawehnerNot sure this was actually documented exactly, but this was just part of the research phase.
Well, the question is, is it good/right to not request the header/status code?
Comment #19
droplet CreditAttribution: droplet commentedWith HTTPStatus in JSTests, we may reduce duplicated / similar tests. (Lazy mode, hehe)
Umm. My first question is:
Can we assume the following conditions?
- Status 200 === Return good content, visible
- Status 400 === Error page, no visible
There're 13 usages in all CORE tests. (Less than I thoughts. It doesn't validate every page requests) IMO, most of them should replace with Text Asserts (even now). I meant we should verify "What We See". (better security)
And there're only 3 usages involved the JSTests:
- drupalLogin & drupalLogout (this is helper function, not testing Login & Logout API)
No idea why it's checking the status code before submission.
- SessionTest
"Status 200" === "session doesn't expire". Is it good?
Note:
- If we request from JS code, it can access the status code from XMLHttpRequest.
- REST-like API should use UnitTests that able to access StatusCode.
- getResponseHeader(), no usages in JSTests.
Comment #20
dawehnerWe could also just simply ignore those assertions.
Comment #21
klausithis should go into core/composer.json
"requires a domain"
{ should be on the same line
we don't use yoda conditions in drupal, so the null should be on the other side.
Yes, I think we should throw a UnsupportedDriverActionException here. Which means we will have to implement our own drupalLogin() method on JavascriptTestBase.
But silently leaving this method empty is a bad idea because then the test will pass although the developer wanted to assert the status code. The test should rather fail instead and tell the developer that this does not work.
Which means this is a potential breaking change for all people using JavascriptTestBase right now.
Which means we need to create a new JavascriptSeleniumTestBase class which has this behavior, while the old JavascriptTestBase stays as is.
Then we would need to document 2 ways how people need to run PhantomJS, which is confusing. And the testbot would have to run PhantomJS in 2 variants?
Comment #22
dawehnerWell, we could silently ignore failures in the status code ... given that they should not have been tested using JTB in the first place.
Comment #23
alexpott@dawehner pinged me for my thoughts on this issue.
Comment #24
dawehnerIdeally we would do that already I'd say: #2827014: Throw an exception when testing status code or response headers in functional JavaScript tests
Comment #25
klausiPatch does not apply anymore, would be nice to have something working here for testing.
Comment #26
klausiAfter having another bad day with PhantomJS having a real browser sounds more and more appealing. Just a reroll for now, I can execute a JS test locally with this.
We should figure out how easy it is to install firefox or chrome on the testbot and how to get PhantomJS running in webdriver mode on port 4444 there.
Comment #27
dawehnerI guess a viable approach would be:
Comment #29
juampynr CreditAttribution: juampynr at Lullabot commentedRe-rolling.
Comment #32
jibranShould we make this configurable?
Comment #33
jibranCan I use it on github.com/jibran/dynamic_entity_reference? Here is a re-roll.
Comment #34
jibranComment #35
droplet CreditAttribution: droplet commented#32,
Yes if we can. Before it's fully integrated into testbot. It's still useful for CORE testing coding.
Core using the system theme for testing, we never know what's going on behind the scene:
https://www.drupal.org/node/2832672#comment-11913964
Comment #36
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI got it to work on PhantomJS, I started the selenium-server and then ran the tests. As long as phantomJS is in your path that is.
I had some problems settings it up mainly because of the supported versions. I added notes on this in the README.md. Still couldn't get it to run in firefox because I haven't been able to figure out yet on how to run 2 versions in parallel as I want to keep my default firefox on the latest version.
Attached patch fixes the issues mentioned by klausi. I split the patch in two, 1 containing only the fixes that are in scope for this issue and 1 with the headers stuff so the patch can pass..
Comment #39
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedForgot the interdiff, attached now. Nothing special changed, a lot of stuff in the composer.lock not sure why it failed here..
Comment #40
jibranRemoved all the unwanted changes in composer.lock form #36.
Comment #41
jibranCreated #2857788: Patch the testbot to start both chrome in webdriver mode and phantomjs in non webdriver mode.
Comment #42
droplet CreditAttribution: droplet commentedIs this line incorrect? I'm running all latest version to test.
keep a comment indicate this is a PhantomJS hack.
Comment #43
dawehnerThis seems to be an unrelated change.
IMHO this should provide some documentation why we are doing this.
Let's ensure that #2827014: Throw an exception when testing status code or response headers in functional JavaScript tests will be committed before that.
Comment #44
jibranPostpone on #2827014: Throw an exception when testing status code or response headers in functional JavaScript tests and #2857788: Patch the testbot to start both chrome in webdriver mode and phantomjs in non webdriver mode.
Comment #45
droplet CreditAttribution: droplet commentedComment #46
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRerolled to update to the latest composer files.
Removed the notes on Selenium 2.x, I got something to work on 3.x. PhantomJS keeps failing, don't know yet. testJavascript() keeps failing in firefox, don't know that either..
If I do get a proper setup with firefox, chrome and phantomjs working I'll update the issue.
Kept the status methods for now to make the patch workable, will rerole after #2827014: Throw an exception when testing status code or response headers in functional JavaScript tests gets committed.
Comment #47
droplet CreditAttribution: droplet commentedIt will, @see #13 logs. Real broswers & PhantomJS using diff way to measure dimensions. My very first patch has a fix, I think we can fix it in a new issue thread also. ( https://www.drupal.org/files/issues/testbots.patch )
Comment #48
droplet CreditAttribution: droplet commentedHeadless Mode In CHROME!!!
https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md
https://www.chromestatus.com/feature/5678767817097216
We really need a roadmap and catch up things :)
Comment #49
dawehnerI recommend you to calm down :)
Comment #50
Mixologichttps://groups.google.com/forum/#!topic/phantomjs/9aI5d-LDuNE phantomJS is going to go unsupported as well.
UNSUPPORTED!
Comment #51
dawehnerNote: There is also some conversation started in #2815199-13: Add tools and scripts for writing and running javascript unit tests
Comment #52
cosmicdreams CreditAttribution: cosmicdreams commentedCame here to also report on headless mode in Chrome (which works in Chrome canary and could be used to fine tune things now) and the planned shutdown of phantomjs. Cool to see you're all in the know.
Given that knowledge how do we cover the gap? Do we just need to change the directives that the test runner uses to launch web pages during testing?
Comment #53
dawehnerThis is tricky now. In a BOF during Drupalcon we talked about using nightwatch, see #2869825: Leverage JS for JS testing (using nightwatch) for a detailed explanation.
Comment #54
droplet CreditAttribution: droplet commentedJust some random thoughts...
I think we need a prediction at the moment. I don't think webdriver will be the future since Chrome Headless come out now. Chrome will push the whole eco forward a big step. webdriver is missing a lot of cool APIs compare to RDP.
https://github.com/cyrus-and/chrome-remote-interface#implementations
http://remotedebug.org/
On the other side, I think a solid API designs is more important NOW. For example,
Codeception (PHP) & CodeceptJS is almost sharing same APIs. It's easier for all developers. Adopting a new rockstar is also easy: https://github.com/Codeception/CodeceptJS/blob/master/lib/helper/Nightma...
Also, take a look at:
https://github.com/laravel/dusk
Comment #55
justafish@droplet Chrome Headless is indeed super cool, but it's not a replacement for Web Driver (which runs on top of it). You still need the Selenium/Web Driver protocol to actually drive the browser and click around etc. See https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md
Comment #56
dawehnerThank you for providing all those links. I have a bit of a problem to see why we should push to use codeception, given that it would require us another HUGE rewrite of all the tests we have. Are our tests actually that simple can we can use the syntax/magic codeception provides? It could be nice.
I agree it offers a lot of nicety, but I believe writing tests is not hard because of the syntax. The trick is to actually know what you want to test.
It also seems to be the case that the actual usage rates of codeception aren't that high (https://www.npmjs.com/package/codeceptjs has 25000 downloads vs. https://www.npmjs.com/package/nightwatch has 225000 downloads)
Comment #57
droplet CreditAttribution: droplet commented@justafish,
You're also correct. WebDriver is one of the communicating channels. It runs Chrome via Chromedriver. Only Safari 10 is supported WebDriver natively.
Chrome:
Selenium -> WebDriver -> Chromdriver -> Chrome DevTools Protocol -> Chrome
Edge
Selenium -> WebDriver -> Edgedriver -> (Simiilar to Chrome DevTools Protocol, or exactly using it) -> Edge
Firefox (Latest version)
Selenium -> WebDriver -> geckodriver -> Marionette proxy -> Firefox
Safari 10
Selenium -> WebDriver -> Safari
I've read a lot of articles in the past and recently. Most of them stated Selenium supporting these drivers development and many bugs around that. They made the WebDriver protocol actually. And some more interesting things, e.g.:
https://news.ycombinator.com/item?id=14240402
And it seems some features able to do with Selenium only, not WebDriver itself.
Comment #58
justafish@droplet You don't need to use Selenium at all when using Nightwatch for Chrome and Edge, the standalone application can be used: http://nightwatchjs.org/gettingstarted#chromedriver
There's even an npm package that's super easy to install https://www.npmjs.com/package/chromedriver
Comment #59
droplet CreditAttribution: droplet commented@justafish,
That just replaced Selenium with another packages only. It's very diff with communicating with browser natively. Whatever, it isn't the point here.
"Chrome DevTools Protocol" vs "WebDriver". "Chrome DevTools Protocol" is better.
To be clarified, I didn't say no to nightwatch.js or any other E2E testings. Just try if we able to find a better and solid workaround for the future and fit into Drupal. If it's a private project or other JS libs, to change a test engine almost seamless usually. But in Drupal, it takes years:
#2809161: Convert Javascript/AJAX testing to use JavascriptTestBase
I asked the maintainer of nightwatch in an issue thread and will see if we have more info.
https://github.com/nightwatchjs/nightwatch/issues/1439#issuecomment-2989...
Comment #60
MixologicIm not sure if this is the place to mention this, but I've been experimenting a little with trying out chrome headless and https://packagist.org/packages/dmore/chrome-mink-driver.
I wasn't actually able to get a test fully working, but I didnt fully attempt to do much beyond changing JavascriptTestBase to use a different mink driver and have chrome in a container.
In any case, I've been spending a lot of time attempting to repurpose some hardware that we have (old db servers, old webnodes etc) so that these can become dedicated testing resources, and Im running into a bug with phanomjs where the network fails out when running on debian in a vmware environment. Which means I cant support the daily testruns on these bots, so I cant save us $$ in repurposing this hardware.
It seems as though my only options are 1. get rid of phantomjs, and replace it with something like headless chrome. 2. figure out how to compile phantomjs 2.5.0 beta for debian jessie, which I've made some attempts, but utterly failed to do (you have to compile qtwebkit, a virtually unsupported dependency first in order to compile phantom). 3. figure out how to use one of the ubuntu binaries of phantomjs.
Anyhow, Im hoping we can come up with some sort of short term solution to doing javascript tests that will allow us to configure a different backend such that either a webdriver solution, or a direct chrome mink solution can work.
Comment #61
droplet CreditAttribution: droplet commented@Mixologic,
It works.
Comment #62
dawehnerLet's keep the discussion focused on using webdriver via selenium here. It solves at least one problem.
The step after that is to use chrome's webdriver implementation on the testbot. This should enable the usecase of @Mixologic!
Once we are there we could then explore the idea of nightwatch/other stuff .
It'd be nice to have some documentation why why need this here ...
We have a dedicated issue for it, don't we?
Comment #63
droplet CreditAttribution: droplet commentedI documented some in Comment #13 patch
#2827014: Throw an exception when testing status code or response headers in functional JavaScript tests
We can reroll all missing things after above issue.
Slightly OFF-TOPIC:
I just realize above package @Mixologic linked is not "webdriver", but binding the API to Chrome devtool API.
Strictly, this package may not an equal conversion of webdriver.
(Note: I can be wrong and very wrong. I've never dug into selenium2/fb php-webdriver source code deeply)
Take an example:
The ->visit() API:
Look at the last 2 lines. chrome-mink-driver package added extra steps for verification, which meant this chrome-mink-driver package creates an unreal environment for the testing.
The human won't wait for everything loaded before their mouse actions and our JS should work on`DOMContentLoaded` and before ideally. (Even though, we may not able to catch the diff between this 2 events. Drupal CORE has very lightweight JS usages and runs tests locally. There're only a few ms between these 2 events. But in Contrib, they can load remote resources)
Overall, it seems not the prime time to use chrome-mink-driver.
Comment #64
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedReroll based on the patch 'javascripttests_with-2775653-46-with-statuscode.patch' which uses behat/mink-selenium2-driver.
Can't get it to run for now locally (errors with cookies) but haven't tried a lot right now, think my Selenium setup is currently broken.
Comment #65
cosmicdreams CreditAttribution: cosmicdreams commentedWith Chrome 59 coming out on June 5 (the day of this comment) Headless Chrome is now in stable.
Here's some guidance from Google Developers that shows how it's used via the command line and programmatically with node: https://developers.google.com/web/updates/2017/04/headless-chrome
Deeply linked in that article is a setup guide for getting Selenium working with headless chrome:
https://intoli.com/blog/running-selenium-with-headless-chrome/
It sounds like we don't think dman's chrome driver library is ready yet so perhaps we should experiment with getting selenium and headless chrome connected?
Comment #66
cosmicdreams CreditAttribution: cosmicdreams commentedAnd here's an issue for gitlab where people are working through similar issues: https://gitlab.com/gitlab-org/gitlab-ce/issues/30876
Comment #67
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRight, my mac just updated chrome to 59 and decided to give it a spin.
With the following:
- Selenium 3.4 (http://www.seleniumhq.org/download/)
- ChromeDriver 2.29 (https://sites.google.com/a/chromium.org/chromedriver/downloads)
- chromedriver in my PATH (FISH: set PATH $PATH /path/to/chromedriver
Starting Selenium with the following:
java -jar selenium-server-standalone-3.4.0.jar
And setting:
$this->minkDefaultDriverArgs = ['chrome'];
... It worked! JSWebAssertTest ran succesfully. Haven't tried other tests yet. Funny thing: it worked headless out of the box and can't figure out how to not run it headless just to see what's going on :)
PhantomJS keeps failing though with some cookie errors:
Firefox complains about the following:
Safari also works! (after enabling the remote control via developer tools) Broke something in my install causing some errors in the test but it starts up. Nice to see it clicking through the test :)
EDIT: I used the latest patch for testing this, nothing changed.
Comment #68
droplet CreditAttribution: droplet commented@michielnugter,
I guess you ran chrome with
--headless
before. And the test takes that instance to run. To check the Activity Monitor :)We can pass more flags to disable plugins and use a custom profile or always open a new instance to test on a cleaner Chrome instance also.
Comment #69
andypostUsing
--headless --disable-gpu
works on Ubuntu with chrome 59Comment #70
droplet CreditAttribution: droplet commentedHeadless Firefox in Windows:
https://bugzilla.mozilla.org/show_bug.cgi?id=1355150
MacOS is WIP:
https://bugzilla.mozilla.org/show_bug.cgi?id=1355147
It's interesting, no Linux so far..Linux landed also:
https://bugzilla.mozilla.org/show_bug.cgi?id=1356681
Comment #72
larowlan#2827014: Throw an exception when testing status code or response headers in functional JavaScript tests is in, one less blocker
Comment #73
dawehnerHere is a reroll I'm using while trying to integrate it into the testbot.
Comment #74
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like Firefox's headless mode lands in v56. Which, at the time of this comment, is the next version of Firefox Stable. Firefox Nightly is at version 57 so we can start testing it if necessary.
Documentation for how to use it with Node.js + Selenium is here: https://developer.mozilla.org/en-US/Firefox/Headless_mode
Comment #75
droplet CreditAttribution: droplet commented#73 is missing DrupalSelenium2Driver file.
Comment #76
jibranAdded DrupalSelenium2Driver file back.
Comment #77
dawehnerThank you @jibran
I've some more changes locally, so I'm assigning the issue to myself.
Comment #78
dawehnerComment #80
MixologicIm getting very close on having a chromedriver container running on the testbots. But I need the patch from #2894482: MINK_DRIVER_ARGS does not support associative arrays in order to test it, so I've combined them here.
Comment #81
dawehnerThank you @Mixologic!!
Let's postpone this issue on the other one then. I posted a new patch over there. This should be easy to fix :)
Comment #82
dawehnerComment #83
dawehnerThis exposes now two additional environment variables:
MINK_DRIVER_ARGS_WEBDRIVER
andMINK_DRIVER_CLASS_WEBDRIVER
Comment #84
GrandmaGlassesRopeManComment #85
dawehnerJust a reroll + small documentation adaption, waiting for #2894501: Allow for separate mink driver settings for Javascript tests to be actually pushed.
Comment #86
dawehnerHere is a proper reroll.
Comment #87
dawehnerComment #92
dawehnerI pinged @Mixologic on slack, so he can use this patch and test it with the testbot infrastructure.
The idea is to start with one test using webdriver and then move over the other one's over time. Last time I talked with @Mixologic this wasn't just a fire and go, as some tests seemed to be broken.
Comment #94
MixologicHey, check that out. https://dispatcher.drupalci.org/job/drupal_patches/39407/testReport/Java...
Green patch in #86 now that I deployed chromedriver on the testbots.
Comment #95
cspitzlayThat's great news. Congrats!
Comment #96
cspitzlayBTW: Webdriver seems to be strict about whether an element is visible if you want to fill in a value or click it.
It requires that it is not hidden, in the view if the page is scrollable and not covered by other elements.
phantomjs OTOH would just do it. I wonder if core's JS tests will show the same issues I was seeing when I made the switch for our own tests...
Comment #97
dawehnerIndeed, I've had annoying times in my life with modals in tests. I think though this makes sense as design limitation, given we test user interaction.
In case we need to still fill in other fields, maybe we could do something like #2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase aka. provide some helpers for hacky tests.
Should we throw some
@trigger_error
as well?Comment #98
cspitzlayComment #99
dawehnerThere is no reason that this is on needs work at this point in time.
Comment #100
xjmFor #97, dawehner asked about BC for
getHtmlOutputHeaders()
. Did it ever work with Phantom? Maybe it should actually raise a fail or exception that that is not allowed in that type of test though? For more explicit debugging.Comment #101
dawehner@xjm
Yes, phantomjs included commands to fetch the HTTP errors as well HTTP headers. Due to webdriver being basically an abstraction over a normal user in a browser, things like HTTP status and headers no longer exist, at least in the view of whoever designed webdriver :)
For the assertions we have right now:
so I guess doing the same makes sense here.
Comment #102
droplet CreditAttribution: droplet commentedanyone mind to review this doc again? Because when I read it I don't fully understand why did it.
I first introduce this for phantomJS only
https://www.drupal.org/project/drupal/issues/2775653#comment-11646983
Comment #103
dawehnerDoes this make it easier to understand?
Comment #104
mtodor CreditAttribution: mtodor at Thunder commentedI checked this a bit since in Thunder we are using WebDriver for a long time with Selenium and Chrome driver. It works fine with few method overwrites. With refactoring here, it looks way better, on first glance - only
$minkDefaultDriverClass
and$minkDefaultDriverArgs
should be provided and that's it. Nice job!There is the only thing that I would like to ask related to this part:
We are setting
$minkDefaultDriverArgs
inJavascriptTestBase::initMink()
, then the parent is called and in the parent after a few up and down function callsgetMinkDriverArgs()
is executed and result of that replaces$minkDefaultDriverArgs
. So my question is, why we don't move defaults in functionminkDefaultDriverArgs()
and return them only ifgetenv('MINK_DRIVER_ARGS_WEBDRIVER') ?: getenv('MINK_DRIVER_ARGS_PHANTOMJS') ?: parent::getMinkDriverArgs();
and so on - is empty. Then it would be more readable and understandable what is going on there. Maybe I'm missing something, is there some reason for not doing that? And also it would be nice thatgetMinkDriverArgs()
returns array instead of string then. That would also remove some checks if empty value fromminkDefaultDriverArgs()
and also assumption that function will return Json string.Comment #105
dawehnerWe (@mixologic, @justafish, @drpal, @dawehner) discussed a bit about JS testing in general
a) We agreed that testing JS in JS is the obvious choice
b) We agreed that we will not be able to get rid of JS testing in PHP given semantic versioning
c) @drpal especially made the point that in order to get the Drupal community into the mindset of using JS having a PHP based testing being around is a bad thing. Once we figured out JS testing we want to actively deprecate to test JS in PHP
d) We agreed on proceeding on the following steps:
Comment #106
GrandmaGlassesRopeMan~
Comment #107
cspitzlayI hope d) 1. means that support for driving a browser with JS enabled will stay around (non-deprecated) even after testing JS in PHP is "actively deprecated". In the tests of the application we have the focus is not on testing JS but in a few spots the JS support is needed.
Comment #108
alexpottI think this is not necessary anymore.
re #97 I don't think we should add an @trigger_error to getHtmlOutputHeaders() since \Drupal\Tests\BrowserTestBase::drupalGet() will call it.
Comment #109
dawehnerI totally think so, and if its just for semantic version reasons :)
@alexpott
Good point. I totally think we can remove the arguments code as well.
Comment #110
MixologicThis LGTM. +1
Comment #111
alexpottI've tried to test this patch and have not been at all successful.
I can run javascript tests with the old
PhantomJSDriver
no problem.If I try to run the new webdriver test
\Drupal\FunctionalJavascriptTests\Tests\JSWebWithWebDriverAssertTest
usingphantomjs --debug=true --webdriver=4444
I have a problem setting cookies... the first one that fails is the xdebug cookie but also the simpletest cookie fails too...If I download selenium server 2 or 3 I can not get either to work with Firefox - this might be because there is no combination that is compatible with the latest Firefox and the PHP webdriver implementation.
Going to try chrome via selenium now.
I think I might be doing something wrong but regardless I think we need better instructions.
Comment #112
alexpottSo chrome worked fine with Selenium 3.
My steps to make it work:
Firefox does not work... here's what I do extra:
(note the null is replaced with a [] - with a null firefox does not even start
This so nearly works but the
findButton
doesn't work because it looks like\WebDriver\Container::webDriverElement
doesn't play nice with what firefox is returning... it expects an array with an ELEMENT key but we have an array with a key likeelement-6066-11e4-a52e-4f735466cecf
Comment #113
Mile23......
Basically these instructions are not correct.
Comment #114
MixologicNeeds issue summary update badly.
Where things are at now is we're aiming to support *both* Mink->gastonjs->phantomjs and deprectate it, alongside adding Mink->webdriver->chromedriver(geckodriver/edgedriver/etc).
Additionally, we don't want to suggest running phantomjs in webdriver mode at all, because we're trying to avoid anybody using phantomjs for anything. Its deprecated abandonware at this point.
geckodriver might not be completely webdriver api compliant. I've installed chromedriver on the testbots, and am using that as the baseline for now.
is the only test that should actually use webdriver as of this patch, all other javascript tests would continue to use phantom until we convert them over. (many of them pass simply by changing the class, others might require some investigation)
Comment #115
BerdirI recently also switched from phantomjs to chromedriver in one of our custom distributions with about 3500 behat assertions. Created about 3 issues/merge requests, the biggest problem was basically that chrome/ChromeDrive are much stricter in regards to visibility of elements when writing into fields/clicking things but the driver does not yet ensure that, so a lot of things fail silently/in unexpected ways. https://gitlab.com/DMore/chrome-mink-driver/issues/42 addresses some of that but there are more cases.
Another tricky problem is timing, Chrome is a lot faster with JS execution and things, so a common problem I had is that ajax was basically finished but the behaviors were still running or elements like ckeditor/dropzone weren't fully initialized yet.
But overall, I'm pretty happy, seeing a lot less random fails now.
Comment #117
dawehnerComment #118
LendudeAhhh I got it to work!
It would be nice if these instructions were the same. The sudo -u _www only applies when you need to run as the _www user. So that might need some additional comment for those that just want to copy/paste the instructions
Also, when I run
./vendor/bin/phpunit -v -c --testsuite functional-javascript
, it throws:Could not read "--testsuite".
Because it's missing the 'core' part.
I followed these steps again, but one essential part is starting the selenium server, so we might want to add the step:
selenium-server -port 4444
Or you just get skipped tests.
Ok so I didn't feel like running the entire testsuite, so I tested with:
./vendor/bin/phpunit -v -c core --filter JSWebWithWebDriverAssertTest
Result:
But it pops a chrome window and runs everything! Pretty cool.
Starting PhantomJS (the old fashioned way, not using webdriver) and running:
./vendor/bin/phpunit -v -c core --filter JSWebAssertTest
Gives the same deprecation notice but works too.
So instead of removing the PhantomJS start instruction would it be better to move that to the ## Functional javascript tests part of the readme and expand it with starting selenium-server?
Comment #119
droplet CreditAttribution: droplet commentedComment #120
LendudeRedid the README, I think it flows a bit better now, but let me know if it needs more tuning.
Edit: the interdiff is a bit weird, interdiff didn't want to play nice for some reason....
Comment #122
Lendudepull before you roll....
Comment #123
dawehnerNice!
That's a much nicer flow! Thank you
Comment #124
LendudeMissed the -c changed, but updated the phpunit commands to be the same now.
Comment #125
jibranDocs look much better now. Thank you!
Comment #126
droplet CreditAttribution: droplet commentedThis is even neater and for every platform than MacOS:
https://github.com/vvo/selenium-standalone
If we run out of time, simply 2 lines:
If we have little time to polish it:
- Add a npm script (install part only). Even a single line adding to package.json
In the future follow up:
- A npm script for config
- A npm script for test runner
Thanks
Comment #127
claudiu.cristeaYesterday I tried to write a JS test for #2921810: Allow TimestampFormatter to show as a fully cacheable time difference with JS and I could not run it with PhantomJS because it seems that PhantomJS has a limitation/bug when the scripts are using
setTimeout()
& Friends. Applying this patch, I managed to run the test with Selenium. So, +1 to see this in.Comment #128
alexpottThe patch looks fantastic and works great with the instructions on OS X. I think we should file a follow up add instructions for Linux and OS X. Plus this issue definitely needs a change record. Once we have those things we should commit. It is fabulous watching the test run in chrome! Nice work everyone.
Comment #129
alexpottMade a start on the CR https://www.drupal.org/node/2941464 - it needs work.
Comment #130
larowlanCan we add a comment here as to why we're doing this?
should we point out that these brew commands are for OSX and provide links for Windows/Linux users. We know they are, but not everyone will.
Comment #131
alexpottI've finished the change record and added the follow-up #2941560: Add testing with webdriver instructions for other operating systems. We still need to added #130
Comment #132
alexpottAddressed #130. I don't think specifically catching the DriverException is required. The next catch is:
Which I think addresses why the test is skipped. Something failed in starting Mink / Webdriver and the test itself has not failed.
Also fixed a minor coding standards issue.
Comment #133
alexpottI just ran through the instructions again and got confused because all but 1 of the tests were skipped. That's because they are legacy. I think we should include instructions about how to force all BTBs to use webdriver.
I've also added docs to phpunit.xml.dist since this is a good place to configure stuff.
Comment #134
LendudeNice additions to the docs, looks great. Some small tweaks to the CR.
Looks ready again.
Comment #135
droplet CreditAttribution: droplet commentedComment #136
alexpottAdding credit for patch review, patch testing and discussion about the wider issue.
Comment #137
alexpottCommitted 6341df3 and pushed to 8.6.x. Thanks!
Moving this back to 8.5.x and going to ask other committers about backporting this to 8.5.x. I think we should as it allows better JS testing and to keep tests in-sync across 8.5.x and 8.6.x.
Comment #140
Mixologicpatch failed because it was already applied.
One word of caution on setting MINK_DRIVER_CLASS -> that will override all mink classes at this point, not just phantomjs->selenium, i.e. all the BTB tests that rely on Goutte will also switch over to whatever that variable is set to.
Comment #141
tstoecklerAwesome work! Glad that we got this in.
Found #2941862: Overriding both Mink driver class *and* args does not work for JS tests while playing around with this. That's a related, but pre-existing bug with JS-testing so should not hold up backport in any way, though.
Comment #142
alexpottDiscussed backporting with other committers. @catch and @webchick are +1.
Committed 10ee6eb and pushed to 8.5.x. Thanks!
Comment #144
jibranWhat is the next step here? Do we have an issue to switch over core JS testing from
PhantomJSDriver
toDrupalSelenium2Driver
on CI?Comment #145
alexpott@jibran swap the existing tests to use webdriver. However some fail so will need adjusting.
Comment #146
MixologicIt's like Christmas all over again. So happy this is in both!
Comment #147
droplet CreditAttribution: droplet commentedOnly 2 or 3 fails in the old day:
https://www.drupal.org/project/drupal/issues/2775653#comment-11646973
not sure if anyone has a powerful machine can help to re-run it again for us or let's find a workaround for everyone:
#2937380: Maintain popular CI presets to improve developer experience
- PhantomJS used a different way to measure windows size. (@see: https://www.drupal.org/files/issues/testbots.patch)
- CKEditor error: #2903623: BigPipe test hardening: check that CKEDITOR exists to prevent script throws error immediately
If you let me pick one, I will close the following issue first for a long run in the future:
#1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS
Comment #148
Mixologic@droplet : I started some conversion work here #2942900: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver, to see how much we can convert for free: my first result shows that 48/63 tests are passing, so there are only 15 tests that need work to convert core entirely.
The JS tests do not require that powerful of a machine as they are currently running serially on the testbots because phantom cannot be run in parallel. i.e. you're not going to see a significant speed boost on another CI system vs locally, other than you can set it and forget it remotely and come back to it later.
Comment #149
jibranDid anyone try this on travis?
Comment #151
heddnShouldn't we updated https://www.drupal.org/node/2116263 as well?
Comment #152
Mixologic@heddn: indeed. Many places need docs updating: https://www.drupal.org/project/drupal/issues/2941560