#131 | interdiff-2942900.txt | 977 bytes | dawehner |
#131 | 2942900-131.patch | 93.06 KB | dawehner |
|
#127 | interdiff-2942900.txt | 776 bytes | dawehner |
#127 | 2942900-127.patch | 93.08 KB | dawehner |
|
#125 | interdiff-2942900.txt | 1.36 KB | dawehner |
#125 | 2942900-123.patch | 92.66 KB | dawehner |
|
#124 | interdiff-2942900.txt | 15.91 KB | dawehner |
#124 | 2942900-123.patch | 107.21 KB | dawehner |
|
#113 | interdiff-2942900-110-113.txt | 871 bytes | martin107 |
#113 | 2942900-113.patch | 91.62 KB | martin107 |
|
#110 | 2942900-110.patch | 97.26 KB | martin107 |
|
#105 | 2942900-104.patch | 90.8 KB | jibran |
|
#105 | interdiff-40d970.txt | 799 bytes | jibran |
#102 | 2942900-102.patch | 90.02 KB | Lendude |
|
#100 | 2942900-100.patch | 89.83 KB | Lendude |
|
#94 | interdiff-90-94.txt | 7.88 KB | Anonymous (not verified) |
#94 | 2942900-convert-js-to-webdriver-94.patch | 89.84 KB | Anonymous (not verified) |
|
#90 | interdiff-89-90.patch | 862 bytes | Anonymous (not verified) |
|
#90 | 2942900-convert-js-to-webdriver-90.patch | 90.34 KB | Anonymous (not verified) |
|
#89 | diff-patches-84-89.txt | 1.87 KB | Anonymous (not verified) |
#89 | 2942900-convert-js-to-webdriver-89.patch | 89.5 KB | Anonymous (not verified) |
|
#85 | part5_js_concurrency.patch | 747 bytes | Mixologic |
|
#85 | part4_harder_conversions.patch | 35.19 KB | Mixologic |
|
#85 | part3_easy_conversions.patch | 38.56 KB | Mixologic |
|
#85 | part2_convert_jstb2wbtb.patch | 10.93 KB | Mixologic |
|
#85 | part1_webdriver_curl.patch | 5.27 KB | Mixologic |
|
#84 | 2942900-convert-js-to-webdriver-84.patch | 89.5 KB | Mixologic |
|
#83 | interdiff-79-83.txt | 8.51 KB | Anonymous (not verified) |
#83 | 2942900-83.patch | 91.22 KB | Anonymous (not verified) |
|
#79 | interdiff-74-79.txt | 2.01 KB | Anonymous (not verified) |
#79 | 2942900-79.patch | 88.87 KB | Anonymous (not verified) |
|
#78 | interdiff-74-76.txt | 1.38 KB | Anonymous (not verified) |
#78 | 2942900-76-actually.patch | 88.23 KB | Anonymous (not verified) |
|
#74 | 2942900-74-actually.patch | 88.23 KB | Mixologic |
|
#72 | interdiff-63-74.txt | 13.55 KB | Anonymous (not verified) |
#72 | 2942900-74.patch | 86.71 KB | Anonymous (not verified) |
|
#72 | interdiff-63-reroll.txt | 7.61 KB | Anonymous (not verified) |
#72 | 2942900-63-reroll.patch | 89.67 KB | Anonymous (not verified) |
|
#63 | interdiff-57-63.txt | 9.99 KB | Anonymous (not verified) |
#63 | 2942900-63.patch | 92.89 KB | Anonymous (not verified) |
|
#61 | interdiff-57-61.txt | 13.62 KB | Anonymous (not verified) |
#61 | 2942900-61.patch | 90.16 KB | Anonymous (not verified) |
|
#58 | 2942900-convert-js-to-webdriver-58.patch | 86.4 KB | Mixologic |
|
#57 | 2942900-convert-js-to-webdriver-57.patch | 87.11 KB | Mixologic |
|
#57 | interdiff-57.txt | 1.1 KB | Mixologic |
#55 | 2942900-convert-js-to-webdriver-55.patch | 86.95 KB | Mixologic |
|
#53 | 2942900-convert-js-to-webdriver-52.patch | 86.95 KB | Mixologic |
|
#53 | interdiff-52.txt | 557 bytes | Mixologic |
#51 | 2942900-convert-js-to-webdriver-51.patch | 86.9 KB | Mixologic |
|
#50 | 2942900-convert-js-to-webdriver-50.patch | 86.2 KB | Mixologic |
|
#50 | interdiff-50.txt | 40.51 KB | Mixologic |
#49 | c1-2942900-selenium-tests-login.patch | 21.46 KB | Anonymous (not verified) |
|
#46 | 2942900-46.patch | 50.17 KB | Lendude |
|
#46 | interdiff-2942900-37-46.txt | 1.28 KB | Lendude |
#37 | interdiff-23.txt | 1.36 KB | Mixologic |
#37 | 2942900-37.patch | 50.17 KB | Mixologic |
|
#30 | 2942900-30.patch | 51.28 KB | jibran |
|
#30 | interdiff-71d77d.txt | 2.47 KB | jibran |
#23 | 2942900-2944291-combined-23.patch | 52.82 KB | jibran |
|
#23 | 2942900-23.patch | 50.16 KB | jibran |
|
#23 | interdiff-375ab7.txt | 7.61 KB | jibran |
#22 | 2942900-2944291-combined-22.patch | 52.52 KB | jibran |
|
#22 | 2942900-22.patch | 49.86 KB | jibran |
|
#18 | 2942900-18.patch | 49.86 KB | Lendude |
|
#17 | interdiff-17.txt | 599 bytes | Mixologic |
#17 | 2942900-convert-js-to-webdriver-17.patch | 55.97 KB | Mixologic |
|
#11 | interdiff.txt | 1.09 KB | Mixologic |
#11 | 2942900-convert-js-to-webdriver-11.patch | 55.73 KB | Mixologic |
|
#9 | interdiff.txt | 50.75 KB | Mixologic |
#9 | 2942900-convert-js-to-webdriver-9.patch | 55.73 KB | Mixologic |
|
#7 | i2942900-convert-js-to-webdriver-7-interdiff.txt | 2.01 KB | Mixologic |
#7 | 2942900-convert-js-to-webdriver-7.patch | 12.01 KB | Mixologic |
|
#5 | 2942900-convert-js-to-webdriver-5.patch | 11.27 KB | Mixologic |
|
#2 | 2942900-convert-js-to-webdriver-2.patch | 639 bytes | Mixologic |
|
Comments
Comment #2
MixologicThis crude patch makes DrupalSelenium2Driver the default class just to see what tests will pass without any work, and which tests will need adjusting.
Comment #3
MixologicComment #5
MixologicThis patch takes a BC breaking shortcut to run all of the javascriptTestBase tests as DrupalSelenium2Driver, and adjusts the breaking tests to use PhantomJS.
(There were 15 failing tests out of 63, with 48 that can be converted right away. We probably should have a "ModernJavascriptTestBase" instead of "LegacyJavascriptTestBase" for BC purposes
Of the 15 failing tests, there were a couple of abstract classes that extended JavascriptTestBase - OffCanvasTestBase, MediaSourceTestBase, and MediaJavascriptTestBase.
All of the tests that extended OffCanvasTestBase were failures, so I made OffCanvasTestBase itself extend LegacyJavascriptTestBase.
The other media tests were a combination of failures and passes, so it was simpler to directly set the $minkDefaultDriver class on those tests.
Comment #7
MixologicHere's one with the missing use statements I forgot.
Comment #8
dawehnerMh, I'm not sure we can actually do that :( Wouldn't this potentially break contrib tests? I wonder what for example @xjm thinks about it
Comment #9
MixologicYah, as I mentioned above:
These were just some preliminary patches to figure out the scope and depth of the conversion requirements.
But what if we inverted the API : Make LegacyJavascriptTestBase into JavascriptTestBase, rename current JavascriptTestBase to WebDriverTestBase and rename all the classes that pass on WebDriver. Then the API is still there, still uses phantom, can be @deprecated, and we can easily convert our existing webdriver ready tests over.
There would just be those 4 weird media tests that dont inherit directly from JavascriptTestBase that would still need the driver class set directly on them (maybe we convert those first)
Perhaps this should be 2 patches, one to wrangle the api around, one to do the renames, but here's my current attempt to do both)
Comment #11
MixologicApparently if one uploads a patch on the form, then makes tweaks, reloads the issue, and re-uploads the same patch filename, it keeps the first patch. These were just some things that leaked through.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commented#11: Exactly, the substitution of files is really a dangerous thing. I have a similar case in #2891911-20: Random fail in Drupal\Tests\locale\Functional\LocaleTranslationUiTest::testStringTranslation. When the patch ends with a fail after this - it's a lucky chance. Much worse, if it does not cause visual problems. Then typing errors or accidental scraps may fall into the core. And your interdiff and conscience will be clear, because you really think that you posted a good patch.
We also have already discovered a useful stability effect from the new engine in #2936122: Find out why JavscriptTestBase occasionally needs a waitForElement on a normal page load. Therefore, the conversion of tests will definitely reduce the number of random failures due to
pressButton(), press(), clickLink(), click()
methods. Woot!Comment #13
MixologicI did a failure analysis of the 15 remaining phantomjs tests that fail, and they fit into the following four categories:
1. They check the response code or headers and they shouldnt, as per https://www.drupal.org/node/2827014 (2 tests)
Drupal\FunctionalJavascriptTests\Ajax\AjaxFormPageCacheTest
Drupal\Tests\action\FunctionalJavascript\ActionFormAjaxTest
2. The test makes assumptions about visibility that is different under chrome than it was under phantomjs (4 tests)
Drupal\Tests\field_ui\FunctionalJavascript\EntityDisplayTest
Drupal\Tests\media\FunctionalJavascript\MediaDisplayTest
Drupal\Tests\node\FunctionalJavascript\TestSettingSummariesContentType
WebDriver\Exception\ElementNotVisible: element not visible
Drupal\Tests\system\FunctionalJavascript\OffCanvasTest
I wasnt totally sure how to categorize this test, as the expected vs actual seems to be equivalent to me.
3. They upload a file as part of the test, which doesnt seem to work like its supposed to (6 tests)
Drupal\Tests\system\FunctionalJavascript\ThemeFormSettingsTest
Drupal\Tests\file\FunctionalJavascript\MaximumFileSizeExceededUploadTest
Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest
This test doesnt always fail.
Drupal\Tests\media\FunctionalJavascript\MediaSourceAudioVideoTest
Drupal\Tests\media\FunctionalJavascript\MediaSourceFileTest
Drupal\Tests\media\FunctionalJavascript\MediaSourceImageTest
4. PhantomJS was hiding real failures, or theres something else wrong with the tests (3 tests)
Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest
Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest
Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest
Comment #14
tstoecklerNice work!
In switching to webdriver for my own testing, I hit a bug in Chromedriver which seems to arise when switching in and out of iframes. See https://bugs.chromium.org/p/chromedriver/issues/detail?id=2198 for the upstream bug. This is not relevant for core, as far as I know, but the test was testing the contrib entity browser module which uses an iframe currently. I was able to get it to work though, by downgrading Chromedriver to 2.33. I built a Docker image for that purpose, as well, based on the upstream selenium/standalone-chrome container: https://hub.docker.com/r/tstoeckler/standalone-chrome/ Not sure if that's useful to anyone, and not sure if there's a better place to leave this info, but I thought this might catch some people following along at home who might be having the same problem.
Comment #15
jibran@Mixologic this is some seriously nice work so thank you. Patch looks solid. I think we need a change notice here.
I have a couple of questions not entirely related to the dev work:
Comment #16
Mixologic@jibran: The testing time improvement is major, and my primary focus here. Phantomjs was never able to be run concurrently, so testing time is significant due to running 63 tests, one at a time, took about 21 minutes. Once everything is webdriver, the tests can be run in parallel, and I was seeing testing times of about 3 minutes for the entire suite for a concurrency of 24.
So now you see why Im motivated to get this work done. It'll save us about 18 minutes per test, which at our current costs of roughly .42/hr, and the fact that we run about 60,000 tests of core per year, well, thats a good chunk of money the DA can save ((.42/60)*18*60000 = $7,560).
The other side effect is that phantomjs is *brittle* and results in random failures, which is disruptive. If we can get on a stack that many, many, many other people use, then we can be more likely to find upstream bugs, like @tstoeckler found in #14
Im not sure what you mean by resource usage. I dont think the webdriver mink classes vs the phantomjs mink classes are materially different, resource wise. If you're planning on running chromedriver with 32 concurrency, then yeah, you need a lot of resources. but otherwise its roughly equivalent.
Comment #17
MixologicI realized that in true backwards compatibility fashion, that we need to keep a proxy of LegacyJavascriptTestBase if any contrib projects or custom modules have for some reason extended that class already.
Comment #18
Lendude@Mixologic++ really great stuff!
Updated the IS with steps that are done and I think are still needed, anything I missed?
So we need the CR (and 'see' to the CR in the deprecation messages) and the follow ups.
Rolled a version of the patch in #17 with --find-copies (no actual changes) that is a little smaller and more clearly shows that WebDriverTestBase is a good copy of JavascriptTestBase with only the needed changes.
similarity index 96%
copy from core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
copy to core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
Comment #19
jibranThanks, @Mixologic that was insight full.
I meant memory usage of PhantomJS and Selenium binaries.
Comment #20
MixologicWe're switching out phantomjs for anything that implements the webdriver protocol, so its not just selenium, but can be standalone chromedriver, geckodriver, edgedriver or selenium, so the testbots just run headless chrome + chromedriver, with no selenium at all. So I dont know that a valid comparison can really be made as we're going from one specific software, to many options/choices.
Phantomjs itself can even run in webdriver mode (with the --webdriver flag) but our mink-selenium2-driver library doesnt seem to support it all that well, (https://github.com/minkphp/MinkSelenium2Driver/issues/258), and I tried it out and it failed on every test attempting to set the cookie with "WebDriver\Exception\UnableToSetCookie:". If that setcookie issue can be sorted out, then in theory there is potentially *no* change to a developers testing resource needs as they could possibly still run phantomjs. (though, I would still advise against encouraging that).
@lendude:
That is super handy! thanks, makes it much easier to review, and I learned a new git option as well.
Comment #21
MixologicI did some research on #2942900-13: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver, point 3, to see if there was a fix for the file uploads. There is, sorta. I opened a followup: #2944291: Upgrade behat/mink-selenium2-driver to 1.3.x-dev
Comment #22
jibranHere is the combined patch to fix the #13.3 and reuploading #18.
Comment #23
jibranUpdated all the tests in #13.3 to use
\Drupal\FunctionalJavascriptTests\WebDriverTestBase
Comment #24
Wim Leers/me cheers from the sidelines!
Comment #25
jibranAdded test to #23 patch as #2944291: Upgrade behat/mink-selenium2-driver to 1.3.x-dev in.
Comment #26
jibranI think we should create rest of follow-ups and commit this patch after agreeing on the naming of the new class.
Comment #27
dawehnerMh, I'm wondering whether
JavascriptWebDriverTestBase
would be a better name overall to indicate it is about JS, but well, I also agree that putting web driver into it is the important idea.Comment #28
MixologicI was thinking that WebDriver could have other uses beyond Javascript testing, like, if we wanted to run a visual regression testing tool, we could power it with the webdriver api to gather browser screenshots.
Comment #29
MixologicAlso:
Maybe, being a testing thing, this goes into 8.5.x ?
Otherwise, I agree with #26, the patch in #23 is a great start.
Comment #30
jibranThanks, for the input.
Comment #31
jibranCreated JavascriptTestBase is deprecated in favor of WebDriverTestBase feel free to improve it.
Comment #33
dawehner@jibran
Do you understand the remaining two failures?
Comment #34
Mixologic@dawehner: Im pretty sure that moving ActionFormAjaxTest to be a WebDriverTestBase and removed the check on the status code, fixed the issue it had in 13.1, but revealed another instance of a 13.2 type failure.
Comment #35
MixologicAlso, maybe we can get Berdir's advice on how to handle the 13.2 type failures, as it appears he had some experience in fixing them for his own suite: https://www.drupal.org/project/drupal/issues/2775653#comment-12401946
Comment #36
jibran@dawehner I have an idea how to fix them but I think we should move
ActionFormAjaxTest
conversion to follow up.Comment #37
MixologicHere's the patch from #30, minus the ActionFormAjaxTest changes.. so essentially #23 with the deprecation fix from #29
Comment #38
MixologicComment #39
jibranLet's create follow-ups and mark it RTBC.
Comment #40
tedbowI was looking at OffCanvasTestBase and created #2946294: Fix race conditions in OffCanvasTestBase which I guess is really follow up to this.
Comment #41
tedbowGreat work on this. but it seems like this issue should wait on #2941560: Add testing with webdriver instructions for other operating systems
It seem like there should be basic documentation on how to use DrupalSelenium2Driver before convert most of our tests. Otherwise any one how wants make a change that will affect any of these tests is going to have a very bad day.
Comment #42
alexpottAt the moment HEAD is failing badly on PHP 7.1-dev and PHP 7.2-dev on javascripttestbase tests - queue up tests to see if this improves that.
Comment #43
alexpott@tedbow for experience the instructions only really get better once the change has been made. That's because people testing in all the different possible environments is better than trying to pre-empt what the problems might be.
Comment #44
alexpottThis needs to be updated for 8.6.x. Also I think we should deprecate JavascriptTestBase too in favour of WebDriverTestBase
Comment #45
tacituseu CreditAttribution: tacituseu commentedRe #42: the reason seems to be that
PHPUnit-FunctionalJavascript
tests are started with--concurrency "36"
in command line, assumed someone was testing something.Comment #46
Lendude#44 addressed
We are @deprecated'ing it, we just can't add the trigger_error yet because it's still in use.
Comment #47
Mixologic@tacituseu nice spot, I had made some changes so that concurrency was set by the number of processors, but it looks like it only works when we're explicitly setting an environment variable upstream.
I've opened https://www.drupal.org/project/drupalci_testbot/issues/2951617 to address it.
Comment #48
MixologicFixed #2951617: concurrency should respect the settings of the yaml file if it is set.. 7.1.x and 7.2.x are back to having 1 concurrency.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch with the passing of the remains red tests. Based on #2 way for simplicity, so certainly is not a candidate for RTBC. It also contains unnecessary changes. But if someone is working on it now, it might be useful. See also #2960784: Document a FAQ for Selenium tests.
Comment #50
Mixologic#49 nerdsniped me something fierce, and I've been spending pretty much every day since drupalcon wrestling with this patch.
So, I incorporated @vaplas's wonderful findings from #49, and rerolled it together with #46, and put the patch through its paces. Needless to say, there were a lot of random failure scenarios, and it took me a while to track them down.
The primary one is that chromedriver does not entirely support having multiple processes all requesting data from it at the same time. There are race conditions where chromedriver will refuse the curl attempt from within the instaclick lib, causing whatever was happening in the test to appear to fail, but only if there just happened to be some overlap.
So one thing I've added here is
core/tests/Drupal/FunctionalJavascriptTests/WebDriverCurlService.php
.It overrides the default curl service that comes with the instaclick driver, by replacing it in the DrupalSelenium2Driver class's constructor.
We need to do this because:
CURLOPT_FAILONERROR
,\WebDriver\Exception\CurlExec Exception
, and the *existing* code in the current CurlService class is broken as its passing the wrong number of arguments to its exception factory (Todo:file a bug upstream)A second thing that I had to do to make these work is that the DragTo class would also fail randomly, very consistently, so I had to override that with retries as well, and make less assumptions about keeping an element to reuse it. Now dragto gets the element from the webdriver server each time it has plans to manipulate it.
If this comes up all green, then what we'll be seeing is *all* of the PhantomJS tests converted to Webdriver.
Comment #51
MixologicAnd because Im impatient, here's the same patch with an accompanying drupalci.yml file that only runs the javascript tests, and allows them to be parallelized.
Comment #53
Mixologic#51 showed us that curl is definitely failing sometimes, I oopsed on a use statement.
Comment #54
MixologicComment #55
Mixologic#50 shows us that we've got passing tests,
#51 shows us that we do indeed need the extra stuff for running them in parallel because curl fails, and
#52 shows us that the parallel fix, fully implemented, drops the test time to about 7 minutes. - the majority of which is just the SettingsTrayBlockFormTest.
I still have a whole mountain of nasty side eye for SettingsTrayBlockFormTest that for some reason needs to check every single block for every single theme to prove that the settings tray works, all in one test.
Anyhow, here's the patch from #50 with the fix from #52.
Comment #57
Mixologic#55 shows us that 3 retries might not be enough, and that if it fails it should probably throw the exception again. Lets see what happens with that.
Comment #58
Mixologicstill had the drupalci.yml in that last patch, because sunday afternoon, and thoroughness, do not go hand in hand.
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commented@Mixologic, titanic work as always!
Super elegant decoration!
I'm one of those who did it :) Sorry! We just wanted to neutralize any random falls that might occur in this test (#2902191: Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks). But I'm sure that we can achieve what we want without dataProvider. I will try to do it in the coming days.
Now I just wanted to be pleased with the #58 result! 🎉🎉🎉
Comment #60
Lendude@mixologic++++++++, wow
@vaplas If you are looking to take out the dataprovider, take a look at
\Drupal\Tests\link\Functional\LinkFieldUITest::testFieldUI
There we sort of made a pseudo-dataProvider using a
yield
to make sure thatsetUp
isn't run for every item in the dataprovider, sincesetUp
usually takes up the bulk of the test time. Maybe it will help, maybe you need setup to be run in this case (although a quick look makes me think it might not).Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commented@Lendude, thanks for this hint, it is really usefull! I'll take this trick into armament.
I combined #57 + #60 + #2946294: Fix race conditions in OffCanvasTestBase, where @tedbow already done big work to improve the stability of this test for Selenium!
Also included fixes for the random fail that @tedbow described in #11.
Also added some WebDriverCurlService refactoring + reinforcement, to fight with StaleElementReference exception. Unfortunately, this is not enough for multiple repeat testing in parallel mode. But it copes well with a single-threaded mode.
Unfortunately, the dataProvider imitation did not give a x20 acceleration (only x2).
Added a @todo to go back to the data provider after the test runs are more lightweight.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedWell, the WebDriverCurlService refactoring was clearly wrong :)
Reverted to #57 version, just added:
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commented:(
#57 looks like simultaneous runs on two PHP versions led to a CI hangup.
#63 also works unstable.
I have little faith in this, but is it possible to update the driver and the browser to the latest versions will somehow improve the situation?
Now using:
chrome=62.0.3202.94
chromedriver=2.33.506092
Latest:
chrome=66.0.3359.117
chromedriver=2.38.552522
Although I did not find any information about any changes that might help :(
Or
instaclick
request implementation is this the only problem?Comment #66
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedWow, @Mixologic reran few tests, and they are green now! Do you plan to tell the secret of this focus?)
Comment #68
Mixologic@vaplas yeah, Im not really sure if there was a drupalci issue that day, or if the tests are just flaky, and we got lucky. I do find it odd that both managed to stall, and then both managed to succeed. We might want to re-run some more a few consecutive times to make sure that we dont have a hidden random fail with this technique.
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedSpecial issue for check selenium tests #2972833: Check Selenium tests stability.
Comment #70
dawehnerHere is just some form of quick review. I'm happy to fix these points tomorrow, if noone beats me.
Note: We are not using phantomjs anymore so we should maybe update the comment too. Running the tests locally said: It it using 1670/1670 by default. I guess the 400 extra pixel reflect that. We should update the comment though.
I really like this check to see whether something changed!
Are you sure we need those? We do already have a check for some element being visible after that.
It would be nice to document why we are resizing things here.
It feels like we don't need to merge these two test functions into one in this issue. This makes it simply harder to review.
Isn't it enough to wait for one of the three fields?
Comment #71
Anonymous (not verified) CreditAttribution: Anonymous commented@dawehner, thanks for review! I will reroll patch after #2971472: SettingsTrayBlockFormTest is taking 5 minutes to run and respond to #70.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedWebDriverCurlService
maybe we can just remove it. While replace on semanticwaitForOffCanvasToClose()
by your hint. Also removed another code, likeDrupalSelenium2Driver::dragTo()
by same reason. Let's see what happens.settings_tray
tests contain many works of @tedbow from #2946294: Fix race conditions in OffCanvasTestBase. Maybe we should decide about them in that issue.Comment #73
Mixologicre #70.6 - that might have been an artifact left over from my troubleshooting of the curl collisions
Nice work on #2971472: SettingsTrayBlockFormTest is taking 5 minutes to run . My side eye has been healed! 5 min 25 sec!! would be interested to see if you set the concurrency to 17 in the drupalci.yaml based off of what you discovered in #2972833: Check Selenium tests stability
Comment #74
MixologicRetesting #72 but with concurrency 17, and also we now have a drupalci.yml file in core, so the patch modifies the existing vs adding a new one.
Comment #76
MixologicHmm. retrying just that one test, plus a 50x version.
Comment #77
MixologicComment #78
Anonymous (not verified) CreditAttribution: Anonymous commentedUnfortunately, in my research was flawed:
It makes the winner's choice a bit random. So I will repeat them with:
usleep(100000);
Also I am planning to check test stability with delay css/js files + delay response from index.php.
#75: Looks like the field was filled before it became 'ui-autocomplete'. Fixed.
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous commentedMeh, wrong patches :(
Comment #80
heddnShould we deprecate w/ a trigger_error JavascriptTestBase since we add WebDriverTestBase?
Comment #81
MixologicThat was suggested in #46, and at the time we decided not to because we werent actually converting everything. But now we have everything, so maybe it makes sense to do so?
Comment #82
Anonymous (not verified) CreditAttribution: Anonymous commentedAt least this trigger will help to detect the leaked PhantomJS tests, until we works on convertion. Done in #2972833-6: Check Selenium tests stability, will be merge here after green result.
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commentedYep, trigger_error helps find 3 new tests (2 Selenium, 1 PhantomJS). This patch from #2972833-9: Check Selenium tests stability. For more performance, we can decouple SettingsTrayBlockFormTest on 3 tests + run tests in revert order. But it seems this will not give a significant profit (~ 1 minute), so that can be done later.
Comment #84
MixologicHeres the above patch without the drupalci carveouts.
Comment #85
MixologicSince this patch is pretty hefty, and does several things, I went ahead and split it up into some smaller chunks to make reviewing easier.
Part 4 is the real heart of this patch. If we are going to deprecate JavascriptTestBase, then we'll probably need a changerecord that points to documentation that gives the contrib developers who are currently using JavaScriptTestBase some guidelines on how to convert their existing tests over to webdriver.
So what we'll likely need is to take everything we've learned in part4 and combine it into a docs page with everything else that vaplas discovered over in #2960784: Document a FAQ for Selenium tests such that we have a guide for developers on how to convert their existing tests, and then we can write a change record to point at that.
Meanwhile we could also make sure that the docs are updated to ensure that installing chromedriver is clear (seems like we have windows/osx/linux instructions but nothing totally in one place.)
I did see a couple of tests in part4 that didnt remove the use statement on the functionalJavascript, but I didn't fix that at this juncture.
Soon. Soon no more phantomjs on the infra.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commented#85: Reading a patch on the parts really brings great pleasure! 🍰
I could not resist the temptation to take the part-2 into a separate issue #2977386: Provide WebDriverTestBase.
Also #2901792: Disable all animations in Javascript testing сan improve the stability of PhantomJS/Selenium tests.
Comment #87
alexpottHow can we move this issue on? Should we be breaking the issue up as per #85 or just committing #84.
I'm in favour of committing #84 as long as we can show that the test infra is running imo we're good to go.
Atm the patch needs a reroll and probably to convert anything new that's added.
Comment #88
jibran+1 for #84.
Comment #89
Anonymous (not verified) CreditAttribution: Anonymous commentedNit reroll after #2916781: Allow off-canvas dialog to be rendered at the top of the page.
It would be more reliable to start from concurency = 1. And then raise it, if the tests work stably. Yes, it is not a very pleasant idea, but switching to the new driver is better to make gradual.
Comment #90
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated after #2407859: Allow theming throbber element.
Comment #92
Anonymous (not verified) CreditAttribution: Anonymous commentedMy bad with interdiff extension, sorry!
Comment #93
LendudeJust some nits really.
++ for going for 15, really the only way to discover if it will hold up, but I would totally get a more reserved approach.
public? can this be protected?
Open a follow up for this and refer to that and not just set a blanket @todo?
no @author. But do we need to reference the original code somewhere? Not really into the whole licensing thing.
missing new line
> 80 characters
> 80 characters
Comment #94
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for review!
#93.1 Agree. But still 15 just to spend as much as possible the tests with concurency :) This number can be changed to 1 during the commit, if we all agree with this step.
#93.2: Done.
#93.3: Indeed! Filed to #2945051-5: Restore AjaxFormPageCacheTest cache hit test coverage.
#93.4: Done (i hope)
#93.5: Done.
#93.6: Done.
#93.7: Done.
Also fixed CS errors.
Comment #95
LendudeThis looks great.
Removing the 'Needs release manager review' tag since that was set when we were trying to do something BC breaking back in #8. We are completely BC now so I don't think we need that anymore.
Removing 'Needs followup' tag because that was set back when we were only doing a partial conversion. We got everything now (wooo!).
Comment #96
alexpottSo this will impact core but in a good way - faster testing and better js testing. One concern I have about committing this right now is that 8.5.x is still open for bugfixes so this might make it trickier to backport and it is going to be disruptive to things pushing for 8.6.x. So one possibility to minimise disruption we'd commit this after 8.6.0 Feature Freeze: Week of July 18, 2018. @Mixologic - thoughts?
Comment #97
MixologicI think the only disruption that might happen is if people are pushing to get something that requires a javascript test, and they get blocked on not being able to set up their environment, as Im not 100% sure we've responded to #41. I know that some folks have gone through the steps, and made changes to the docs, but I havent re-assessed them.
That being said, I think *that* risk is low. It doesnt seem as though there is a tremendous amount of velocity on things that need these tests, and it seems like we've only needed to chase changes to head in this patch a few times in four months or so.
If I were to referee this change, I'd say that the random phantomjs failures are at least as disruptive as any potential disruptions we'd be introducing.
Either way, I'll go have a look at the docs issues to see where things stand.
Comment #98
alexpottJust committed some patches with additional JS tests - #2831944: Implement media source plugin for remote video via oEmbed and #2828528: Add Quick Edit Functional JS test coverage let's see if this breaks.
Comment #100
LendudeReroll was needed:
is now this
Comment #102
LendudeHmm somehow the quickedit changes didn't come through when I pulled, *shrug*. Reroll number 2!
changes:
And removed the @todo to this issue.
Comment #103
jibranBack to RTBC after the reroll.
Comment #105
jibranChanged the base class for
Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
Comment #107
LendudeHmm related or not .... I don't know. Seems pretty stable so far otherwise.
Comment #109
jibranNew JTBs in HEAD
core/modules/file/tests/src/FunctionalJavascript/FileFieldValidateTest.php
core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
core/modules/media/tests/src/FunctionalJavascript/MediaJavascriptTestBase.php
core/modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php
core/modules/media/tests/src/FunctionalJavascript/MediaViewsWizardTest.php
Comment #110
martin107 CreditAttribution: martin107 as a volunteer commentedRerolled.
Comment #111
Lendude@martin107++
I think you missed
class FileFieldValidateTest extends JavascriptTestBase
And try rolling the patch with --find-copies, it reduces the size of the patch a little bit.
PS. If you are at DevDays, I'm down in the sprint dungeon
Comment #113
martin107 CreditAttribution: martin107 as a volunteer commentedThanks .. at some point I must show up to a Drupal event, but not today.
Comment #114
LendudeThanks!
Comment #115
borisson_rtbc++, great work!
Comment #116
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedComment #117
alexpottCrediting everyone who made constructive comments on the issue. Thanks everyone!
Comment #118
alexpottCommitted f55d974 and pushed to 8.6.x. Thanks!
Comment #120
alexpottWe missed something :(
If you run a test without a webdriver service running you get this message:
Not that helpful :(
Comment #122
dawehnerI'm looking into that quickly
Comment #123
alexpottActually I was wrong the message doesn't mention phantomjs. The message is:
However this shows us something else. The old message was super helpful. This one is not. Let's try to make it better before going forward.
Comment #124
dawehnerHere is a better error handling. I am wondering whether including the original message is still helpful.
Before
After
Comment #125
dawehnerGood try. This is the same but with the right interdiff/patch file.
Comment #126
alexpottWhilst testing the patch I've realised that the default driver args for webdriver are wrong which makes config-less startup with chromedriver more difficult than it should be.
should be
The current code results in URLs like
http://localhost:4444//session
and errors:If we fix that then running tests using chromedriver should only need you to start chromedriver like this:
chromedriver --port=4444
and not have to message about with setting MINK_DRIVER_ARGS etc.
Comment #127
dawehnerThank you @alexpott!
Comment #128
alexpottWe have something that needs to be understood. The test runs as a result of the commit failed :( see https://dispatcher.drupalci.org/job/php-7.2-apache_mysql5.5/1499/console
Comment #129
alexpottI think this message would be better as
The test wasn't able to connect to your webdriver instance. Ror more information read core/tests/README.md.\n\nThe original message while starting Mink: {$e->getMessage()}
I discussed the message with @dawehner and @gábor-hojtsy.
If #128 turns out to be nothing and this ends up back at rtbc i'm happy to fix this on commit.
Comment #130
Mixologic#128 is the result of an untested scenario in recent drupalci changes, namely "what happens when core commits a drupalci.yml file, and then reverts it right away"
So, somehow drupalci was sent the new drupalci.yml file with the concurrency of 15, but was testing the revert commit, which was phantomjs. I'll look into how this happened on the PIFT side of things, but that explains the timeouts/failures.
Comment #131
dawehnerHere is an updated message.
Comment #132
LendudeRor => For
Comment #133
Lendude@alexpott offered to fix the typo on commit, RTBC assuming this comes back green.
Comment #134
dawehnerYou know english has all kind of myths, maybe this is one of them.
Comment #135
alexpottSecond time lucky. Committed 27740c2 and pushed to 8.6.x. Thanks!
Comment #137
Mixologic🎆💥 YES💥 🎆
Comment #138
Lendude@here++
awesome work!
Comment #139
jibranYay!!! to ✂️💵💵💵💵💵
Comment #140
andypostIt was commited to 8.6 so change record needs update
Comment #141
dawehnerYeah! Congratulations
I added a follow up for some better experience: #2983777: Improve the user experience of WebDriverTestBase
Comment #142
LendudeWe missed updating the comment in drupalci.yml about concurrency, so another follow up #2983781: Update drupalci.yml concurrency comment to match the new reality of FunctionalJavascript
Comment #144
jhodgdonDocs were not updated with this change. I opened
#3029813: Testing topic documentation is way out of date
to remedy that situation.