Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Over in #2232861: Create BrowserTestBase for web-testing on top of Mink we got step one in, BrowserTestBase.
Now on with step 2 - adding a JavaScript driver.
Proposed resolution
* Use a phantomjs mink driver
* Let people install phantomjs on their server, its a tool like mysql/php
* Use the mink driver in a JavascriptTestBase
* Add a simple test: toolbar
Remaining tasks
* Motivate the testbot team to look at #2580007: Add phantomjs2 to the testrunners
* Review this issue
* profit
User interface changes
None
API changes
New base class for JavaScript testing
Comment | File | Size | Author |
---|---|---|---|
#265 | 2469713-265.patch | 2.73 KB | dawehner |
#233 | 2469713-3-233.patch | 16.12 KB | alexpott |
#233 | 230-233-interdiff.txt | 1003 bytes | alexpott |
#230 | 2469713-3-230.patch | 15.9 KB | alexpott |
#230 | 228-230-interdiff.txt | 1.25 KB | alexpott |
Comments
Comment #1
larowlanComment #2
chx CreditAttribution: chx at Tag1 Consulting commentedThe taxonomy overview page is not your best choice: it certainly breaks but it's the backend that breaks as it tries to pager a drag-and-drop form and that is ... not working well as it can be expected. We filed a solution to this to #242324: Going to page 2 on "list terms" page doesn't display additional terms . (Corporate interests end here.)
In my personal opinion, tabledrag is a good idea but also you might want the new filter on admin/extend because it's woefully complicated as far as I know but ask nod_ whether I remember correctly.
Comment #3
phenaproxima+1 for a TableDrag test case. It's such a widely-used JS feature that it would be incredibly convenient to have a general test for it in core :) I also like @chx's idea of testing the module filter, since it's a totally different kind of JS interaction.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedSee #237566-122: Automated JavaScript unit testing framework for a tabledrag test that could borrowed here.
Comment #5
grom358 CreditAttribution: grom358 commentedInstead of another base class (JavaScriptTestBase) why not have pluggable driver provider to BrowserTestBase. I know registerSessions() is there so a subclass could register other mink sessions, but plugins would allow for using any mink driver people wanted.
Though for core we need to agree on a javascript capable driver to include into the testing infrastructure. (eg. PhantomJS?)
Comment #6
Wim LeersComment #7
dawehnerhttps://github.com/dawehner/MinkPhantomJSDriver looks quite interesting but well, the chain of dependencies require guzzle 6, so this would be fun first to port it over.
Maybe something similar could be a way forward.
Comment #8
dawehnerDoes that actually mean we wanna use it? We could talk with phantomjs directly and implement our own driver for phantomsj or try out Sahi
While working on this issue I saw the following message: #2563821: Implement Twig_Loader_String for ourselves
Comment #10
dawehnerThis time with --binary, mh, I can't upload the file:
Comment #11
jibranFunny thing I set it up, phantomjs, recently on previousnext internal project for JS testing and ran into same problem with status code issue. I was planing to write a blog post on it set up and all that but procrastination won. I had an interesting conversation with @grom358 related to this. Found some chat logs.
Comment #12
jibranIt is 52M because it includes all the binaries from https://github.com/previousnext/phing-phantomjs/tree/master/bin. On internal CI we run composer install before starting testing and then run phantomjs executable using phing from https://github.com/previousnext/phing-phantomjs/blob/master/build.xml
Comment #13
dawehnerhttps://github.com/minkphp/Mink/issues/654 could be pretty interesting, given that it would allow us to not require to use selenium.
Why is that important? Selenium for example doesn't give you access to status code, while phantomjs would do.
Comment #14
pfrenssenSelenium is very complicated in that regard. One way to work around the HTTP status code problem by executing HTTP requests in the browser using Javascript, as an example see http://cgit.drupalcode.org/paddle_selenium_tests/tree/pages/Kanooh/Paddl...
Having extensive experience with Selenium I would vote against it as a default solution. It is very slow, resource heavy and prone to random failures.
Comment #15
dawehnerYeah its pretty crazy, so do you suggest we take part in producing a native PhantomJS driver for mink and use that afterwards?
Comment #16
pfrenssen@dawehner I am very much in favour for that!
Comment #17
dawehnerWhile talking with @jaesin via tests we realized that making it swappable currently is a first good step: #2573165: Step 1.5: Make the driver swappable more easy.
Comment #18
dawehnerThis work here is just experimental and we certainly should figure out #2573165: Step 1.5: Make the driver swappable more easy. first.
Comment #21
dawehnerComment #22
dawehnerThis now actually works quite well. Using ";" on cookie values seems not that trivial as you think it is. Let's use ":", it at least cannot really conflict with the way how cookies are stored internally
Comment #27
dawehnerComment #29
dawehnerAdding a related issue on the testrunner site ... for now this can't work as phantomjs isn't there. If you would check the code and run phantomjs already, you could execute javascript, click on stuff etc. all in the browser.
Comment #30
dawehnerThis now also works.
Comment #31
Wim Leers<3
Comment #33
dawehnerComment #34
elachlan CreditAttribution: elachlan commented#2580007: Add phantomjs2 to the testrunners needs some love and a review.
Comment #35
dawehnerStep 1.5 worked fine, until we tested it :)
Comment #37
dawehnerHere is a "fun" subissue
Comment #38
elachlan CreditAttribution: elachlan commentedSo now that is RTBC, are we just waiting on #2580007: Add phantomjs2 to the testrunners?
Comment #39
dawehnerYeah that one would block this issue as well!
Comment #40
dawehnerHere is a reroll.
Thank you @webchick to commit step 1.75.
Comment #42
dawehnerTotally flabbergasted at the moment.
Comment #45
pfrenssenIsn't this a bug in Mink? Why should we have to do a request before being able to set a cookie?
Is it needed to wait a full second for this? Since PhantomJS is headless, shouldn't this be instant? I always think that we should avoid to wait in tests, but it is possible that this is an asynchronous action and that there is no other way.
Crufty bits.
Comment #46
pfrenssenJust playing around with it a bit. It works great, this is so much fun!
Comment #47
dawehnerYeah I guess 1000 milliseconds is way too much, maybe we could go with less. It would be great to see what other similar test code is using.
Oh yeah I totally think so, but I'm not sure how to fix it yet.
We can obviously drop it.
As a follow up we could try to figure out how to make the usage of mink easier.
Comment #48
pfrenssenThis was causing me some trouble. On my system the global temporary folder is not writeable by the webserver.
Shall we move this to
sites/default/files/simpletest/browsertestbase-templatecache
?Comment #49
benjy CreditAttribution: benjy at PreviousNext commentedIf we want the system tmp folder we should use sys_get_temp_dir() although storing things in the global tmp folder might cause issues when running multiple tests at once, caching in the simpletest folder per test might be better.
Comment #50
dawehnerWorking on the issue summar
Comment #51
elachlan CreditAttribution: elachlan commentedTest bot team has created a branch for the code and we have reviewed it.
What is the next step? Does it need to go into production for us to test this code?
Edit: They created the branch in issue #2580007: Add phantomjs2 to the testrunners.
Comment #52
andypostDelivered #2580007: Add phantomjs2 to the testrunners
now patch needs re-roll
Comment #53
elachlan CreditAttribution: elachlan commentedQueuing the latest patch for testing.
Comment #59
elachlan CreditAttribution: elachlan commentedIt's beautiful :')
Comment #60
dawehnerMh, damnit this wasn't everything which we need ... @elachlan any idea already?
Comment #61
elachlan CreditAttribution: elachlan commentedNope.
PhantomJS started ok and the port is correct.
We didn't patch for #48/#49, it could be permissions. I have no idea though. Its failing in testJavascript().
Comment #62
dawehner@isntall is currently helping us with that. Thank you so much!
Comment #63
alvar0hurtad0Patch #46 applies
Comment #64
isntall CreditAttribution: isntall at Drupal Association commentedI ran a test using my latest attempt to have phantomjs run in the background and patch 2469713-46.patch.
https://dispatcher.drupalci.org/job/staging_simpletest_job/320/
phantomjs was running in the background and there were still the 2 failures.
Comment #65
elachlan CreditAttribution: elachlan commentedThe failure:
In the function "testJavascript", the line:
Are we able to get more error information?
Comment #66
larowlanNote that over in #2114887: Maximum nesting level when attaching comment field to User. I had to make some changes to BTB to recognize base urls, perhaps that the issue here too?
Comment #67
elachlan CreditAttribution: elachlan commentedWere the changes committed?
Comment #68
dawehnerWell, I think that @isntall and the fact that the phantomjs process stops to run at some point, is a clear reason for the problems we see.
Comment #69
elachlan CreditAttribution: elachlan commentedTrying what @larowlan suggested. Not sure if this is all that was needed.
Comment #70
elachlan CreditAttribution: elachlan commentedComment #71
dawehner@elachlan
Always please provide an interdiff, this makes it easier for people to see what you changed.
Comment #72
elachlan CreditAttribution: elachlan commentedHere is a new patch which takes all the changes to browser test base done in the other issue. As well as an interdiff.
Comment #73
dawehnerI'm sorry, but this change was needed due the phantomjs nicety/driver niceties.
These two changes are wrong as well, sorry
Comment #81
dawehnerRerolled the patch
Comment #82
dawehner:(
Comment #84
elachlan CreditAttribution: elachlan commentedDoes BrowserWithJavascriptTest need to extend from JavascriptTestBase?
Comment #85
dawehnerWell yeah I think it should!
Sadly the toolbar one also failed ...
Comment #86
elachlan CreditAttribution: elachlan commentedOne problem at a time, are you able to resubmit with the change?
Comment #87
dawehnerNot in this night, feel free to do it ...
Comment #88
elachlan CreditAttribution: elachlan commentedChanged BrowserWithJavascriptTest to extend from JavascriptTestBase.
Comment #90
elachlan CreditAttribution: elachlan commentedSorry, didn't do the patch right. Interdiffs are hard :(
Comment #92
elachlan CreditAttribution: elachlan commentedI actually accidentally didn't make the change. I am really sorry.
Comment #93
elachlan CreditAttribution: elachlan commentedComment #102
larowlanparent::setUp inits mink, which looks for the templatecache, which we haven't created yet
Comment #103
elachlan CreditAttribution: elachlan commentedOutput from test. We are getting somewhere.
Comment #105
elachlan CreditAttribution: elachlan commentedThe failure is caused by phantomjs being disabled in #2580007: Add phantomjs2 to the testrunners.
The Maintainers will re-enable it and we should be able to test it again.
Comment #107
Mixologic#2580007: Add phantomjs2 to the testrunners Is in now, added a test to see if it works.
Comment #109
elachlan CreditAttribution: elachlan commentedFailed because
Comment #110
Mixologicyep. for some reason daemon, despite being in the php-base container, wasnt there. we've bandaided it by putting it in the .yml as an apt get install. But the test was re-ran and should actually overwrite the results that elachlan is referring to. (third test on #102)
Comment #111
elachlan CreditAttribution: elachlan commentedCongratulations :)
Comment #112
larowlanoh yeah!
Comment #113
elachlan CreditAttribution: elachlan commentedBut also, we need more work on this.
Comment #115
larowlanelachlan - are you working on this? I can - drop in on irc if you want to chat
lee
Comment #116
larowlanComment #117
larowlanComment #118
elachlan CreditAttribution: elachlan commentedI guess we need to catch those and display meaningful output in the results?
Comment #119
larowlanargh
We need a polyfill https://github.com/ariya/phantomjs/issues/10522
Comment #120
dawehnerCan't we just install a more or less recent version of phantomjs?
Comment #121
elachlan CreditAttribution: elachlan commentedWe can use phantomjs2. Not sure if the driver supports that though.
I really think we should use NPM to install phantomjs2.
I am suggesting this because it would better support the move in #2609560: Base DCI containers off official containers.
Comment #122
dawehnerI always used the version 2 of it locally, so that should be alright.
Comment #123
pfrenssenI'm also always using PhantomJS 2.0. I've read in the issue queue on Github that there are "some problems" with it, but I haven't encountered them yet. Also the 2.0 release was already made in January of this year, so I'm assuming that it has been further developed in the meanwhile, even though there doesn't seem to be any release made after that. The last commit on the project was 12 days ago so it is definitely still alive.
Comment #124
benjy CreditAttribution: benjy at PreviousNext commentedI wonder if we should provide an abstraction on JavascriptTestBase for anything that requires a wait()?
This would allow us to be consistent with our waits per different action, across all of our tests. That way, when we figure out that we only had to wait 50ms for a resizeWindow call, we can update it once and all our tests benefit?
Just a thought,
$this->getSession()->getDriver()->
is quite verbose, how about a__call
on BTB that allows us to do this:$this->isVisible()
and proxy any methods that exist directly to the driver?EDIT: We could also use @method to get PHPStorm support on
__call
for the driver methods.Comment #125
pfrenssen@benjy, using wait() is a bad practice and can be avoided in almost all circumstances. It is usually very clear what you are waiting on. In most cases you are waiting for an element to appear in the DOM or vanish from it, or you might be waiting for a certain javascript variable to reach a certain value. You can implement a loop (typically called a
spin()
in this context) that polls the required state change at regular intervals (typically every 50-100ms) and returns as soon as the desired state is reached. In order to prevent deadlocks the spin function will accept a timeout parameter and will throw an exception when this timeout is reached, terminating the test with a failure.So let's implementing a spin() method and use it instead of waiting :)
Comment #126
pfrenssenI wanted to implement a spin() function but wasn't able to run the tests any more with the latest patch:
The reason for this is that the PHP process isn't allowed to write to the global /tmp folder on my system. Let's instead use the temporary folder that BrowserTestBase provides. That is guaranteed to work on all systems, and its also in line with how our other test frameworks handle this situation.
Comment #127
dawehnerGood idea. Now we just need to fix the phantomjs binary.
Comment #128
elachlan CreditAttribution: elachlan commentedI fixed it the other week, just waiting for a review.
Comment #129
benjy CreditAttribution: benjy at PreviousNext commentedYeah, it certainly felt a bit like an anti-pattern, what you suggested sounds good to me!
Comment #130
dawehner@elachlan Oh I was just blind for a while. Thanks a lot!
Comment #131
pfrenssenShall we make the host and port number for the PhantomJS process configurable? I guess this can be useful in some cases. How shall we do that? Source it from an environment variable?
Comment #132
dawehnerWe could put it into an environment variable but set it by default into our
phpunit.xml.dist
file. In general we should try to reduce unnecessary additional required environment variables, and just make it possible to override it.Comment #133
tunicYes, this is needed for example when using a CI tool with several projects. You can configure each project to use a different PhantomI JS instance on its own port , so projects can be tested simultaneously.
Comment #134
Wim LeersNote that once we have this, the BigPipe module will able to have proper tests, i.e. it won't have to simulate the behavior of its JS.
Comment #135
dawehnerAssigning to me while debugging the failure.
Comment #136
dawehnerComment #137
elachlan CreditAttribution: elachlan commentedSetting to Needs Review to run tests on latest patch.
Comment #139
hussainwebRerolling #126.
Comment #140
elachlan CreditAttribution: elachlan commentedTest Failed:
Edit:
Side note
Comment #142
MixologicAn errant print_r($pageSize); seemed to cause the last patch to bail/fail.
Comment #143
MixologicComment #145
MixologicI really should know better than to manually edit a patch.
Comment #146
MixologicAnd this is actually how an interdiff is made. IIRC.
Comment #147
dawehnerDo you mind open up a follow up for now?
Comment #148
elachlan CreditAttribution: elachlan commentedComment #150
MixologicOkay, I think I see what might have gone wrong here.
The testbots have to run N tests at a time in parallel. Im not sure how mink and phantomjs handle multiple requests to the same port, and if there is some state stored in phantom JS that has to be retrieved across multiple connections to 8510, but if I change the toolbar test to be in the 'javascript' group, and do a repeat 2, the tests fail. If I do not repeat, or run them on their own, they pass.
Are we sure that phantomJS is going to be able to handle multiple simultaneous requests and responses? Do we need to spawn a separate phantomjs process per test?
Comment #151
elachlan CreditAttribution: elachlan commentedI see, Did a quick google:
http://stackoverflow.com/questions/28822262/does-phantomjs-share-memory-...
I think we need to implement multiple tabs, with a tab per test.
Comment #152
MixologicFrom the same page:
Tests will need to do things like log users in and out and be isolated from each other. Tabs cannot work.
Comment #153
elachlan CreditAttribution: elachlan commentedSo we would have to create a phantomjs instance per test?
Or do we change the middle layer to have some sort of queue system?
Comment #154
elachlan CreditAttribution: elachlan commentedIsn't mink supposed to handle that?
Comment #155
MixologicI have no idea, but before we make any further changes to the testbots, this needs to work in a local environment with concurrency dialed up. run-tests.sh has a --repeat functionality in conjunction with --concurrency that should do the trick. If you can get that working locally, let us know what you've done - if it requires spawing new phantom js processes for each test or whatever works.
Comment #156
elachlan CreditAttribution: elachlan commented@dawehner you are better positioned to explain this.
Comment #157
dawehnerI'm now trying to basically start one phantomjs process per javascript test. Its the only thing which will keep us sane IMHO.
Comment #158
dawehner@longwave, @Mixologic and @dawehner talked on IRC about this issue
We agreed to split up the process into several steps:
for each test, to getting rid of phantomjs and using selenium + selenium grid etc., but its a discussion which can be done independently from it. The important thing is to get javascript tests itself running at some point.
Comment #159
elachlan CreditAttribution: elachlan commented@dawehner, sounds good.
I guess if we create a job type with a queue, then we can add the concurrency later. This could be done similar to how we currently do it, but we just launch a certain number of daemons based on the parameters.
Comment #160
dawehnerWell yeah, let's play around with ideas/solutions once we have serial testing working.
Comment #161
dawehnerGiven that javascript tests are so fundamental different I propose to make an additional testsuite for javascript functional tests.
This will also make #2659100: Allow run-tests.sh to run just the javascript Functional tests potentially easier.
Comment #162
dawehnerHere is the interdiff
Comment #163
Wim LeersYay, progress! Awesome work :)
I had to read this three times to see the difference: from semi-colons to colons.
Why this change?
->wait()
… that's a rather unfortunate precedent for our very first JS test :P<3 <3 <3
Comment #164
dawehnerPure 100% fun, see https://github.com/minkphp/Mink/pull/675
Basically, PhantomJS doens't handle cookie urlencoding as other stuff. Without it, the special cookies we set will be thrown away on the browser level.
Well, browser windows don't resize instant :)
Comment #165
dawehnerComment #166
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commented@dawehner you might want to consider a custom wait function that doesn't wait an arbitrarily long time. http://stackoverflow.com/questions/22291945/implementing-waitfor-functio...
Comment #167
dawehnerSome continues work, while trying to get the tests running on the testbot
Comment #169
dawehnerThe current steps blocking this issue are: #2670978: Allow to run just specific types when running all tests and #2580007: Add phantomjs2 to the testrunners
Comment #170
dawehnerJust a reroll
Comment #171
isntall CreditAttribution: isntall at Drupal Association commentedThe patch in comment #170 does not apply.
Comment #172
dawehnerRerolled in a minute
Comment #173
kostyashupenkoComment #176
dawehnerLet's do a reroll, now that
--types
is in.Comment #177
dawehnerFixed some autoloader issue.
Comment #178
isntall CreditAttribution: isntall at Drupal Association commentedDCI should now support phantomJS tests.
Comment #179
dawehnerNice @isntall Thanks a ton!
Comment #180
benjy CreditAttribution: benjy at PreviousNext commentedWhy not just Javascript? If we ever had anything like JS unit tests, they'd likely end up in a folder like my_module/tests/mocha/xyz.js
Comment #181
dawehnerWell, my thoughts have been, if we add mocha / karma etc. these would be different kind of tests than the functional tests we have now. Much like we separate the unit tests from the kernel and the functional tests in PHP, IMHO we should try to keep a similar kind of structure for anything javascript related. Yes, you would not run any other kind of javascripttests with phpunit but at least giving it the right name is not the worst idea.
Comment #182
almaudoh CreditAttribution: almaudoh commentedDoesn't look like anything is blocking this now...
Comment #183
pfrenssenI don't know a better way of doing this. Isn't this out of our control, i.e. due to a limitation in Mink or PhantomJS?
We should address this one second wait.
Nitpick: there are different capitalizations used of "javascript", "mink" and "phantomjs" throughout the patch. Let's look up the official spelling :)
Functional tests
Run the functional tests.
Assigning to me, going to address this after lunch.
Comment #184
pfrenssenOh the patch is rolled against 8.0.x? Indeed testing is exempt from the rules against adding new features. Then it applies :)
Comment #185
pfrenssenAddressed my remarks. I researched the workaround for visiting the front page before a cookie can be set, and it turns out it is required to do this by the W3C WebDriver specification, so it is perfectly fine to do this. I've added a comment with more information. I also updated the README so the instructions are now correct for running the entire functional test suite. I also went through the patch with my finest comb to straighten out any last wrinkles.
This looks pretty ready to me now :)
Comment #186
jibranCan we use CssSelector here instead?
Comment #187
pfrenssenSure! Good suggestion @jibran, since these two tests will serve as examples CssSelector will make it much friendlier for people new to JS testing.
Comment #188
dawehnerThank you @pfrenssen
Comment #189
pfrenssenConverted XPath to CSS selectors. Also provided some helper methods to make writing tests much more developer friendly. I wouldn't try to put a full suite of helper methods in now, we can expand on this in followup tickets.
Looking at how the code reads now CSS selectors are indeed so much more friendly to read. I've been writing XPath expressions for so long that I sometimes forget how alien and unfriendly these are for people that are not used to them.
Comment #190
pfrenssenComment #191
pfrenssenWeird I just commented but my session got logged out. Apologies if this comes up double.
Made some small cleanups to the CSS selectors.
Comment #192
MixologicNot sure why, but the testrunner is *not* finding the JavaScript tests.
Comment #193
dawehnerAll of those should land in
BrowserTestBase
Comment #194
jibran+1 to #193.
Comment #195
pfrenssenGreat idea, moved them to BrowserTestBase.
Comment #196
dawehnerThat one is a bit weird, because this is exactly what the symfony css component is doing.
Comment #197
pfrenssen@Mixologic, I checked the
phpunit.xml.dist
for any clue why the tests are not running on DCI but it all seems OK. Strange.Comment #198
pfrenssenAddressed #196.
Comment #199
MixologicOkay, just to clarify, currently the way drupalci is handling these phantomJS tests is to run run-tests.sh a second time with just the js tests because they cannot have concurrency.
Is what the latest test shows in https://dispatcher.drupalci.org/job/default/97850/consoleFull
So, right now, none of these javascript tests are running on drupalci.
Comment #200
dawehnerThis fixes it.
Comment #202
dawehnerThere we go, a small problem in the latest changes from @pfrenssen
Comment #203
pfrenssenGreat! It works!!
Comment #204
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedVery excited for JTB. Great work all involved.
Comment #205
jibranYay!!!
Comment #206
kim.pepperLong time follower of this issue. Thanks for all the great work everyone!
Comment #207
tstoecklerJust to save people some clicking, here's the proof from https://dispatcher.drupalci.org/job/default/98370/console:
Comment #208
alexpottPatch didn't apply to 8.2.x where it needs to go first - plus a couple of coding standards fixes - yes we're going to remove
@file
docblocks but until then core should be consistent.As my changes are extremely minor leaving at rtbc.
Comment #209
dawehnerThank you alex! You are right, I changed my templates in storm already for custom code, so I forgot to add this back for core work.
Comment #210
alexpottTests are still working on 8.2.x... https://dispatcher.drupalci.org/job/default/98893/console
Comment #211
alexpottCommitted 3436dfd and pushed to 8.1.x and 8.2.x. Thanks!
Looking forward to having a test for #states in core :)
Comment #212
alexpottThe postgres fail on #208 was an unrelated random fail that was cause by a patch that has now been reverted.
Comment #213
alexpott@xjm suggested we should tell people about this in the release notes - on the whole I agree although I have some reservations around testbot resource utilisation, just how experimental this is and how DrupalCI/run-test.sh reports fails from PHPUnit based tests.
Comment #217
alexpottHad to roll this back because contrib tests are broken :( see https://dispatcher.drupalci.org/job/default/98960/console
Comment #218
alexpottSo let's merge in #2687731: \Drupal\FunctionalJavascriptTests\JavascriptTestBase::initMink has the wrong return value so that's not a separate issue.
Comment #219
alexpottShouldn't the ->wait call use the method on the test base? Or what is the point of the method on the test base?
Comment #220
pfrenssenComment #221
alexpottComment #222
alexpottHere are the changes that needed applying due to the reviews since #208.
I wonder if
JavascriptTestBase::wait()
should become::assertJsCondition()
and that assertion should take a timeout to wait for it so be true? And we should default this to 0?Comment #223
pfrenssenOn the one hand,
::wait($timeout, $condition)
matches the function that is used in other WebDriver implementations, so this is familiar for people that have prior experience with writing browser tests (e.g. Selenium, Mink, or even test frameworks in other programming languages).On the other hand, wait() encourages bad practices such as hardcoding a waiting period of N seconds for some action to occur. This is common in test code but it is never the correct solution, since you are usually waiting for a specific change to occur to the DOM. Waiting longer than strictly necessary is just a waste of time, and it can also introduce random failures if the action randomly takes longer than expected.
I am in favour of renaming this to
::assertJsCondition()
. Defaulting to a timeout of 0 is not a good idea though, changes made by JS in the DOM are not instantaneous. They are usually very fast, but on heavy pages it might take a few milliseconds and then the assertion will fail. I always use a 1000ms timeout for simple DOM actions since on some third-party services (e.g. BrowserStack) the tests run so slowly that even the simplest action might take 100ms or more.Comment #224
benjy CreditAttribution: benjy at PreviousNext commentedSo we never got the spin discussed in #125? That seemed like the best approach of all, maybe worth re-visiting?
Comment #226
pfrenssen@benjy, the wait() method performs the spin, it is implemented in the webdriver, it will typically poll every 50ms for the JS condition to become true. The method will return as soon as the condition becomes true or the timeout expires.
Comment #227
benjy CreditAttribution: benjy at PreviousNext commentedAh OK, I was confused when you said "I always use 1000ms timeout" because it sounded like it would always wait for that long, but in actual fact, that's only in the worst case, most times it will be much quicker.
Comment #228
alexpottHere's the ->wait() changed to an assertion - it also always us to have a decent log message rather than lots of asserting true is true.
Sample output when made to fail... (by changing assertTrue to assert False)
Comment #229
alexpottProof from the last test run that the tests have run...
Comment #230
alexpottLet's have a test that proves the assertJsCondition can fail...
Comment #231
pfrenssenLooking good! Just one little thing...
The $message parameter should be documented.
Comment #233
alexpottHere's a patch with the missing docs added.
I have no idea why #230 failed :( Passes locally for me.
Comment #235
MixologicThe phantomjs process was not staying alive for some reason. I've adjusted how it keeps itself in place, so those failures above were a result of the phantomjs process not existing.
We deployed a fix, and phantomjs should actually be there now.
Comment #236
MixologicComment #237
pfrenssenFor me it looks good now, but I can't RTBC this.
Comment #238
pfrenssenActually, since this was already committed, and I have not participated in the new version of the patch it is probably OK for me to RTBC this.
Comment #239
alexpottCommitted 5a223b9 and pushed to 8.1.x and 8.2.x. Thanks!
I've committed this because it's not runtime code and we learn more by having this committed than not.
Comment #243
alexpottReverted because the phantomjs going missing issue is not resolved... https://www.drupal.org/pift-ci-job/215899
Comment #244
xjmSo is this actually RTBC or blocked?
Comment #245
xjmComment #246
dawehnerSee #2580007-104: Add phantomjs2 to the testrunners for the current state.
Comment #247
xjmSo I guess it should be postponed until that is fixed?
Comment #248
nevergone CreditAttribution: nevergone commented:'(
Comment #249
alexpott#2580007: Add phantomjs2 to the testrunners is done... The only way we're going to find out is committing this.
Comment #250
alexpottDiscussed with @catch - we think this should be eligible for commit because it was an 8.1.0 target, it was held up in improvements to DrupalCI and has been committed several times already, it is a new test API will not break runtime.
Comment #251
alexpott3rd time lucky?
Committed 94fb7c6 and pushed to 8.1.x and 8.2.x. Thanks!
Comment #254
dawehnerShameless plug: #2702747: Add javascript unit testing
Comment #255
alexpottShameless plug: if you are looking for a how to run tests locally https://www.chapterthree.com/blog/javascript-testing-comes-to-drupal-8
Comment #256
xjmThis issue is missing a change record. There is an existing one with related info that also needs to be updated at: https://www.drupal.org/node/2469723
Comment #257
BLACKBERRY_54CA CreditAttribution: BLACKBERRY_54CA commentedComment #258
dawehnerComment #259
jibranAdded #2711963: Modernized ToolbarIntegrationTest.
Comment #260
dawehnerWe have one now, but of course we can expand it
Comment #261
dawehnerComment #262
xjmAdding back a title prefix since this keeps confusing me by being open. :)
Comment #263
webchickGuys. :( We really need some docs on how to write these tests. :( ATM the best guide is "use the source, Luke" and we're blocking great UX improvements like #2421427: Improve the UX of Quick Editing single-valued image fields on that. I got an offer on Twitter from someone to help write these, and the best I could come up with was the out of date change record, a non-d.o resource, and a list of classes that sub-class BTB. Help. :(
Comment #264
xjmIt looks like @dawehner had drafted https://www.drupal.org/node/2716803 (attached to this issue) and it just hadn't gotten reviewed/published. I published it now, so that should help some. Maybe others can add some things there.
Some API topic or handbook developer guide docs would be really helpful for sure.
Comment #265
dawehnerHere is a patch for core.api.php, which itself is horrible out of date, as it still describes old simpletest.
Comment #269
elachlan CreditAttribution: elachlan commentedI am trying to implement Javascript testing for extlink in #2587531: Convert Automated Tests to BrowserTestBase and I am running into issues.
Is there no better documentation? Could someone have a quick look and see if I am doing anything obviously wrong?
I'll update the documentation once I get it all working and fill in any blanks that weren't too obvious to me.
Comment #270
almaudoh CreditAttribution: almaudoh commentedThe patch at #265 is quite helpful, but it seems it hasn't been committed yet.
Comment #271
dawehner@almaudoh
Do you want to post the patch in a new issue so we can close this one?
Comment #272
xjm@dawehner's patch is in #2810621: Improve core.api.php documentation about browsertests and javascript tests. I think this is still open for review of the CR.
Comment #273
Gábor HojtsyUpdated both https://www.drupal.org/node/2716803 and https://www.drupal.org/node/2469723 (especially the later) by removing outdated info, crosslinking them and linking to the PHPUnit initative.
Comment #274
dawehner@Gábor Hojtsy Thanks a ton