Problem/Motivation

Over in #2232271: [Meta] Use Behat for validation testing we're proposing adding Behat to enable JavaScript testing.
But as a first step we don't need Behat to utilise the Selenium/Sahi/Zombie/PhantomJs drivers, we need Mink.

Proposed resolution

  1. Create a BrowserTestBase that is run on top of Mink.
  2. Use the Goutte driver (pure php) but build the functionality in a driver-agnostic fashion.
  3. Port some tests as a proof of concept.

This will open the door for step 2: Add JavaScriptTestBase with an alternate session implementation.

The pitch

In the Drupal 8 cycle we have invested a lot of time into writing tests. We are now writing unit tests for our logic and acceptance tests for our scenarios, but what about our frontend (Javascript) testing?

Originally Drupal's WebTestBase evolved from SimpleTest but over the years it has morphed and transformed into a mutant beast of difficult to maintain custom code.

Whilst we've been off on our island, the PHP community at large have been working together on solving the same problems we've been adding additional band-aids and bolt-ons to SimpleTest for.

One such solution is Mink.

Say hello to Mink

Many of us are familiar with Behat's gherkin syntax but you may not be aware that the real power of Behat is powered by Mink. Most of the built in step-definitions are added by the Mink extension, which is a Behat Extension that wraps Mink.

The core of Mink is the DriverInterface which allows a swappable back-end for functional testing.
Much of the custom code we have in SimpleTest can be handled by existing functionality in Mink, but in a back-end independent manner.

Since Early April 2014, we have been working on a new functional test base that uses phpunit as the runner and Mink as the browser.

At present in WebTestBase when a test calls $this->drupalGet() the implementation is hard-wired to use curl.

This means that we are limited in our web-testing to things that Curl can handle. As a result we have some interesting work-arounds for JavaScript/Ajax handling (we are looking at you drupalPostAjaxForm()).

With BrowserTestBase architected on top of Mink, we maintain a Mink session for browser interactions. The default implementation of this uses the Goutte driver, which is simple web-scraper that wraps Guzzle and the Symfony DomCrawler component. This abstracts away the browser implementation to the various Mink drivers.

So how does this help Drupal?

  • We get rid of loads of custom-code for parsing/manipulating the dom and just use widely adopted implementations.
  • We get rid of loads of custom Curl handling code and let Goutte and Guzzle worry about it.
  • Any issues we find strengthen those code-bases. We've already ported Goutte to Guzzle 4 - i.e. Goutte 2.0. We've found and fixed bugs in Guzzle 4. PIE for the win.
  • We can move towards real JavaScript testing

Wait how does this help JavaScript testing?

So imagine JavaScriptTestBase extends BrowserTestBase but instead of using the Goutte driver in our session, we instead use the Selenium, Sahi or Zombie drivers - these drivers understand JavaScript - they are either headless browser implementations or they control real browsers.
Or in code terms.

$button = $this->getSession()->getPage()->find('css', '#my-button');
$button->mouseOver();
$button->press();
$this->getSession()->dragTo(CssSelector::toXPath('#my-target'), CssSelector::toXPath('#my-destination'));

Future issues

A) HttpTestBase - Work on creating HttpTestBase that uses Guzzle direct but doesn't need the Mink/DomCrawler elements - for things like Rest and Rss tests - where no browser is required
B) META - Create a meta listing each of the un-converted tests and create issues for each un-converted test
C) Javascript - we recommend PhantomJS as its a real headless browser built on top of WebKit - its just a binary and there are composer libraries so we could foresee-ably use it without any significant overhead on our testing infrastructure.

User interface changes

None

API changes

BrowserTestBase is built on top of Mink and PHPUnit so the asserts are different to WebTestBase. Form submits are handled by Mink so instead of drupalPostForm() and drupalPostAjaxForm() new tests that extend this class will use submitForm() instead. If the Mink driver supports AJAX it will handle submitting form using AJAX.

How to run

$ cd core
$ sudo -u www-data SIMPLETEST_BASE_URL=http://d8.dev ./vendor/bin/phpunit --testsuite functional
# Or with run-test
$ sudo -u www-data php ./core/scripts/run-tests.sh --url http://d8.dev --class "Drupal\Tests\simpletest\Functional\BrowserTestBaseTest"

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Major because automated tests are important, but Drupal currently lacks the ability to test javascript code. Not critical because Drupal has never tested javascript code, and this has historically not been considered to be a blocking issue.
Unfrozen changes Unfrozen because it affects automated tests.
Disruption Not disruptive. It adds a new base class which can be used to write browser based tests. Some changes were made to the way Drupal recognizes whether it is running as part of an automated test, but this does not affect existing code.
CommentFileSizeAuthor
#261 interdiff.txt6.6 KBpfrenssen
#260 2232861.260.patch67.97 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,344 pass(es). View
#260 258-260-interdiff.txt5.71 KBalexpott
#258 interdiff.txt466 bytespfrenssen
#258 2232861-258.patch67.5 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,223 pass(es). View
#255 interdiff.txt513 bytespfrenssen
#255 2232861-255.patch67.79 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,218 pass(es). View
#254 2232861-254.patch68.13 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,220 pass(es). View
#251 2232861-251.patch68.13 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2232861-251.patch. Unable to apply patch. See the log in the details link for more information. View
#236 interdiff.txt8.54 KBpfrenssen
#236 2232861-236.patch68.72 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,985 pass(es). View
#232 interdiff-2232861-226-232.txt4.93 KBdaffie
#232 2232861-232.patch68.63 KBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,681 pass(es). View
#228 interdiff.txt18.82 KBpfrenssen
#226 2232861-226.patch68.82 KBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,636 pass(es). View
#222 220-222-interdiff.txt1.77 KBalexpott
#222 2232861-db.222.patch68.32 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,299 pass(es). View
#220 2232861-db.220.patch68.27 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,294 pass(es). View
#220 218-220-interdiff.txt700 bytesalexpott
#218 2232861-db.218.patch68.29 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,303 pass(es). View
#218 217-218-interdiff.txt2.55 KBalexpott
#217 2232861-db.217.patch68.18 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,287 pass(es). View
#217 213-217-interdiff.txt7.24 KBalexpott
#213 interdiff-2232861-208-213.txt988 bytesdaffie
#213 2232861-213.patch65.6 KBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,302 pass(es). View
#208 interdiff-2232861-202-208.txt1.38 KBdaffie
#208 2232861-208.patch65.78 KBdaffie
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,300 pass(es), 4 fail(s), and 0 exception(s). View
#202 2232861-202-review-do-not-test.patch87.25 KBdaffie
#202 2232861-202.patch1.9 MBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,226 pass(es). View
#199 2232861-199-review-do-not-test.patch87.57 KBdaffie
#199 2232861-199.patch1.9 MBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 104 pass(es). View
#198 2232861-197.patch1.9 MBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 104 pass(es). View
#198 2232861-197-review-do-not-test.patch87.49 KBdaffie
#196 2232861-3.196.patch1.9 MBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,229 pass(es), 3 fail(s), and 0 exception(s). View
#196 2232861.196-to-review-do-not-test.patch86.6 KBalexpott
#196 175-196-interdiff.txt6.4 KBalexpott
#175 2232861-2.174.patch1.9 MBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,916 pass(es), 1 fail(s), and 0 exception(s). View
#175 2232861.174.review-do-not-test.patch83.35 KBalexpott
#175 152-174-interdiff.txt6.17 KBalexpott
#152 interdiff-143-152.txt562 bytesdaffie
#152 phpunit-mink-review-152.patch1.9 MBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,771 pass(es). View
#152 phpunit-mink-review-152-do-not-test.patch78.03 KBdaffie
#151 BrowserTestBaseTest.png74.02 KBdaffie
#143 phpunit-mink-review-143.patch1.9 MBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,722 pass(es). View
#143 phpunit-mink-review-143-do-not-test.patch78.03 KBhussainweb
#137 phpunit-mink.9cdc0fe.patch1.9 MBphenaproxima
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.9cdc0fe.patch. Unable to apply patch. See the log in the details link for more information. View
#137 phpunit-mink-review-do-not-test.patch78.03 KBphenaproxima
#136 Screen Shot 2015-01-27 at 7.27.12 am.png351.38 KBbenjy
#133 interdiff.txt3.56 KBbenjy
#124 phpunit-mink.aad8038.patch1.9 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,464 pass(es). View
#124 interdiff-124.txt1.69 KBgrom358
#124 2232861-124-do-not-test.patch78.03 KBgrom358
#122 phpunit-mink-122.patch1.02 MBpcambra
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,129 pass(es). View
#122 interdiff.txt2.16 KBpcambra
#118 phpunit-mink-118.patch1.02 MBpcambra
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,140 pass(es), 4 fail(s), and 0 exception(s). View
#114 interdiff-105-109.txt7.2 KBgrom358
#111 interdiff-105-109.patch7.2 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-105-109.patch. Unable to apply patch. See the log in the details link for more information. View
#109 2232861-109-do-not-test.patch69.48 KBdaffie
#109 2232861-109.patch1.02 MBdaffie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,901 pass(es). View
#105 phpunit-mink-105-do-not-test.patch74.37 KBhussainweb
#105 phpunit-mink-105.patch1.02 MBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,872 pass(es). View
#103 interdiff-2232861-103.txt12.1 KBgrom358
#103 phpunit-mink.e542ac6-do-not-test.patch74.33 KBgrom358
#103 phpunit-mink.e542ac6.patch1.02 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,465 pass(es). View
#98 phpunit-mink.a265c93-core-only-do-not-test.patch75.16 KBgrom358
#97 interdiff-2232861-97.txt8.38 KBgrom358
#97 phpunit-mink.a264c93.patch1.02 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,557 pass(es). View
#89 phpunit-mink.e387bd4.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.e387bd4.patch. Unable to apply patch. See the log in the details link for more information. View
#86 phpunit-mink.b830771-do-not-test.patch75.12 KBgrom358
#86 phpunit-mink.b830771.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.b830771.patch. Unable to apply patch. See the log in the details link for more information. View
#84 phpunit-mink.a0add69-do-not-test.patch75.12 KBgrom358
#84 interdiff-2232861-79.txt2.98 KBgrom358
#84 phpunit-mink.a0add69.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.a0add69.patch. Unable to apply patch. See the log in the details link for more information. View
#81 phpunit-mink.e907b6c-core-only-do-not-test.patch88.69 KBjibran
#80 interdiff-2232861-72.txt14.53 KBgrom358
#79 phpunit-mink.e907b6c.patch1.02 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,828 pass(es). View
#76 phpunit-mink.fd21438.patch1.02 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,817 pass(es). View
#72 phpunit-mink.72.patch1.02 MBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,835 pass(es). View
#72 mink-core-only.do-not-test.patch75.49 KBlarowlan
#67 phpunit-mink.ba9fb4a.patch1.02 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,767 pass(es). View
#64 phpunit-mink.a353dd4.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#62 phpunit-mink.2db5b17.patch1.03 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#60 interdiff.txt2.43 KBmoshe weitzman
#60 phpunit-mink.f69b6fd.patch1.02 MBmoshe weitzman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,961 pass(es). View
#55 phpunit-mink.200a8ad-do-not-test.patch91.56 KBjibran
#52 phpunit-mink.200a8ad.patch1.02 MBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,152 pass(es). View
#49 phpunit-mink.88bed73.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,155 pass(es), 1 fail(s), and 0 exception(s). View
#48 phpunit-mink.f9bd883.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,198 pass(es), 1 fail(s), and 0 exception(s). View
#46 phpunit-mink.d2259db.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 48,672 pass(es), 8,963 fail(s), and 7,081 exception(s). View
#44 phpunit-mink.5efdc5f.patch1.03 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,196 pass(es), 2 fail(s), and 0 exception(s). View
#42 phpunit-mink.660d1ce.patch1.03 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,153 pass(es), 4 fail(s), and 0 exception(s). View
#36 phpunit-mink.33ffe5b.patch1.02 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,038 pass(es), 2 fail(s), and 0 exception(s). View
#31 phpunit-mink.158bbd4.patch1.56 MBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#30 phpunit-mink.773f298.patch1.56 MBgrom358
#28 2232861-28-phpunit-mink.patch1.49 MBnick_schuch
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2232861-28-phpunit-mink.patch. Unable to apply patch. See the log in the details link for more information. View
#25 mink-testing-2232861.7ba406f.patch907.77 KBgrom358
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,896 pass(es). View
#23 mink-testing-2232861.78024e2.patch897.59 KBgrom358
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,359 pass(es), 1 fail(s), and 0 exception(s). View
#18 mink-testing-2232861-green.18.patch1.17 MBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,276 pass(es). View
#18 mink-testing-2232861.core-only.18.do-not-test.patch136.69 KBlarowlan
#18 interdiff.txt2.89 KBlarowlan
#15 mink-testing-2232861-green.whole-biscuit.patch1.17 MBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,789 pass(es), 45 fail(s), and 0 exception(s). View
#15 mink-testing-2232861.core-only.do-not-test.patch137.65 KBlarowlan
#7 Screenshot 2014-05-09 07.41.11.png68.26 KBlarowlan
#3 Screenshot 2014-04-09 07.26.09.png204.1 KBlarowlan
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

sun’s picture

Since I've worked on many similar simpletest issues, I'd like to prevent you from running into insanity ;-)

For the sake of making progress, I think it would be best to try whether an initial step here can be architected and implemented (sort of) in an "opt-in" way; for example, by extending WebTestBase with a new class that uses Mink instead of cURL.

That will make all existing tests run against the current code (for now), and we can focus on getting the Mink based version to work first.

larowlan’s picture

Roger that

larowlan’s picture

Making progress here

larowlan’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +API change

Bumping priority based on some discussions and adding API change now that approach is clearer and there is a patch

sun’s picture

larowlan’s picture

woohoo Goutte with Guzzle 4 was merged so this is open again.
https://github.com/fabpot/Goutte/commit/2735d2400aab84e5c26bc1c854478d5f...

larowlan’s picture

Have a working implementation on Guzzle 4
Will keep running tests in #2083741: IGNORE: Patch testing issue till less fails.

rcross’s picture

Just curious if there is any consideration for using http://codeception.com/ (or any reasons against it)

Codeception seems to allow for a most consistent interface for both acceptance, functional and unit tests and is utilising popular libraries underneath including mink, phpunit, etc.

benjy’s picture

+1 for Codeception, i'd be interested in hearing more thoughts on this.

sun’s picture

Sorry, but Codeception is off-topic here. This issue is about making a first big leap towards leaving the Simpletest sphere.

Anything beyond this step is a pipe-dream at this point. So let's stay focused here, please.

(You can, however, create a dedicated issue with a proper proposal, outlining the pros and cons, in order to discuss that idea.)

larowlan’s picture

Issue summary: View changes
larowlan’s picture

Issue summary: View changes
larowlan’s picture

Issue summary: View changes
jibran’s picture

It's green yay!! #2274167-96: IGNORE: Patch testing issue. Now reviews.

larowlan’s picture

Title: step 1: Re-architect WebTestBase on top of Mink » Step 1: Create BrowserTestBase for web-testing on top of Mink
Issue summary: View changes
Status: Active » Needs review
FileSize
137.65 KB
1.17 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,789 pass(es), 45 fail(s), and 0 exception(s). View

Here's where we're at, we about-faced from converting everything and introduced a BrowserTestBase that's based on Mink and Goutte.
We moved over some existing tests.
The plan from here is to
a) work on refining BrowserTestBase - posting here for reviews
b) Build a trait containing the common hunks between WebTestBase and BrowserTestBase, we didn't sub-class off WebTestBase because we didn't want to rely on it and we eventually want to get rid of it (we've marked it @deprecated) - so the patch as it stands contains lots of duplication that belongs in a trait.
c) split off sub-issues to add the required libraries we need in their own issues
d) work on creating HttpTestBase that uses Guzzle direct but doesn't need the Mink/DomCrawler elements - for things like Rest and Rss tests - where no browser is required
e) Create a meta listing each of the un-converted tests
f) Create issues for each un-converted test

Attaching 'core-only' and 'whole biscuit' patches (which includes all the vendor changes).

larowlan’s picture

Here's a start on cleanup we need to do

  1. +++ b/composer.json
    @@ -3,8 +3,21 @@
    +      "type":"git",
    +      "url": "https://github.com/larowlan/MinkGoutteDriver.git"
    

    We can revert this too, now https://github.com/Behat/MinkGoutteDriver/commit/428df9dd75c56b0d76a5308...

  2. +++ b/composer.json
    @@ -3,8 +3,21 @@
    +    {
    +      "type":"git",
    +      "url": "https://github.com/larowlan/guzzle.git"
    +    }
    

    We can revert this now and return to using Guzzle as released, they merged our PR

  3. +++ b/composer.json
    @@ -3,8 +3,21 @@
    +    "behat/mink-goutte-driver": "dev-master",
    +    "fabpot/goutte": "dev-master",
    

    We can pin both of these now

  4. +++ b/composer.json
    @@ -19,7 +32,7 @@
    -    "guzzlehttp/guzzle": "4.1.*",
    +    "guzzlehttp/guzzle": "dev-master",
    

    We should open a dedicated issue for updating Guzzle, 4.1.3 includes @grom358's upstream fixes (https://github.com/guzzle/guzzle/releases/tag/4.1.3)

  5. +++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
    @@ -146,7 +147,41 @@ public function testExport() {
    +   * export does not, therefore this is work around to testing textarea value.
    

    is *a* workaround

  6. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -448,7 +534,7 @@ protected function assertNoBlockAppears(Block $block) {
    -    return $this->xpath('//div[@id = :id]', array(':id' => 'block-' . $block->id()));
    +    return $this->getSession()->getPage()->find('css', '#block-' . $block->id());
    

    <3 - so much cleaner

  7. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1188,43 +1276,15 @@ protected function tearDown() {
    -    if (!isset($this->curlHandle)) {
    -      $this->curlHandle = curl_init();
    -
    -      // Some versions/configurations of cURL break on a NULL cookie jar, so
    -      // supply a real file.
    -      if (empty($this->cookieFile)) {
    -        $this->cookieFile = $this->public_files_directory . '/cookie.jar';
    -      }
    -
    -      $curl_options = array(
    -        CURLOPT_COOKIEJAR => $this->cookieFile,
    -        CURLOPT_URL => $base_url,
    -        CURLOPT_FOLLOWLOCATION => FALSE,
    -        CURLOPT_RETURNTRANSFER => TRUE,
    -        // Required to make the tests run on HTTPS.
    -        CURLOPT_SSL_VERIFYPEER => FALSE,
    -        // Required to make the tests run on HTTPS.
    -        CURLOPT_SSL_VERIFYHOST => FALSE,
    -        CURLOPT_HEADERFUNCTION => array(&$this, 'curlHeaderCallback'),
    -        CURLOPT_USERAGENT => $this->databasePrefix,
    -      );
    -      if (isset($this->httpauth_credentials)) {
    -        $curl_options[CURLOPT_HTTPAUTH] = $this->httpauth_method;
    -        $curl_options[CURLOPT_USERPWD] = $this->httpauth_credentials;
    -      }
    -      // curl_setopt_array() returns FALSE if any of the specified options
    -      // cannot be set, and stops processing any further options.
    -      $result = curl_setopt_array($this->curlHandle, $this->additionalCurlOptions + $curl_options);
    -      if (!$result) {
    -        throw new \UnexpectedValueException('One or more cURL options could not be set.');
    -      }
    

    out you go

  8. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1356,52 +1441,49 @@ protected function curlExec($curl_options, $redirect = FALSE) {
    +  public function curlHeaderCallback(AbstractTransferEvent $event) {
    
    @@ -2694,8 +3180,32 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
    +      'complete' => ['curlHeaderCallback', RequestEvents::EARLY],
    +      'error'    => ['curlHeaderCallback', RequestEvents::EARLY],
    

    We can rename this now, to something more appropriate

  9. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1644,10 +1727,20 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
    +            // Guzzle only supports strings for submitted values.
    +            $post = $post + ($extra_post ?: array());
    +            foreach ($post as $key => $value) {
    +              if (is_bool($value)) {
    +                $post[$key] = '1';
    +              }
    +            }
    +            $out = $this->drupalPost($action, 'application/vnd.drupal-ajax', $post, $options, $upload);
    

    This can go - any test that relies on this is ajax and belongs on JavaScriptTestBase

  10. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1667,6 +1764,7 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
    +          $this->drupalSetContent($out, $this->getUrl());
    

    I think it's called setRawContent now

  11. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1816,7 +1912,7 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
       protected function drupalProcessAjaxResponse($content, array $ajax_response, array $ajax_settings, array $drupal_settings) {
    -
    +    // @todo larowlan refactor around Mink
         // ajax.js applies some defaults to the settings object, so do the same
         // for what's used by this function.
         $ajax_settings += array(
    @@ -1903,8 +1999,16 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr
    
    @@ -1903,8 +1999,16 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr
           }
         }
         $content = $dom->saveHTML();
    -    $this->setRawContent($content);
    -    $this->setDrupalSettings($drupal_settings);
    +    $this->drupalSetContent($content);
    +    $this->drupalSetSettings($drupal_settings);
    +    // Hack this into the mink session. When we use a real JavaScript driver we
    +    // can remove this nastiness.
    +    $reflection = new \ReflectionClass($this->getSession()->getDriver());
    +    $method = $reflection->getMethod('getCrawler');
    +    $method->setAccessible(TRUE);
    +    $crawler = $method->invoke($this->getSession()->getDriver());
    +    $crawler->clear();
    +    $crawler->addContent($content, 'text/html');
    

    This can go to, ajax forms aren't responsibility of BrowserTestBase » defer to JavaScriptTestBase issue

  12. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1923,24 +2027,56 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr
    +  protected function drupalPost($path, $accept, array $post, $options = array(), $files = array()) {
    +    /** @var \GuzzleHttp\Client $guzzle */
    +    $guzzle = $this->getSession()->getDriver()->getClient()->getClient();
    +    $cookies = [];
    +    if ($session_id = $this->getSession()->getCookie($this->session_name)) {
    +      $cookies[$this->session_name] = $session_id;
    +    }
    +    $request = $guzzle->createRequest('POST', url($path, $options + array('absolute' => TRUE)), array(
    +      'body' => $post,
    +      'cookies' => $cookies,
    +      'allow_redirects' => FALSE,
    +      'timeout' => 30
         ));
    +    if ($accept) {
    +      $request->addHeader('Accept', $accept);
    +    }
    +    $request->addHeader('Content-Type', 'application/x-www-form-urlencoded');
    +    if (preg_match('/simpletest\d+/', $this->databasePrefix, $matches)) {
    +      $request->setHeader('User-Agent', drupal_generate_test_ua($matches[0]));
    +    }
    +    foreach ($files as $name => $file) {
    +      $request->getBody()->addFile(new PostFile($name, fopen($file, 'r')));
    +    }
    +    try {
    +      $response = $guzzle->send($request);
    +      $this->drupalSetContent($response->getBody());
    +      $this->responseCode = $response->getStatusCode();
    +    }
    +    catch (RequestException $e) {
    +      $response = $e->getResponse();
    +      if ($response == NULL) {
    +        $this->fail($e->getMessage());
    +      }
    +      else {
    +        $this->drupalSetContent($response->getBody());
    +        $this->responseCode = $response->getStatusCode();
    +      }
    +    }
    +    $this->verbose('POST request to: ' . $path .
    +      '<hr />' . $this->content);
    +    return $this->content;
    

    This can go into HttpTestBase - needed for Rest etc, non-form implementations

  13. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1950,16 +2086,17 @@ protected function drupalPost($path, $accept, array $post, $options = array()) {
       protected function getAjaxPageStatePostData() {
    +    // @todo larowlan refactor around Mink
         $post = array();
         $drupal_settings = $this->drupalSettings;
         if (isset($drupal_settings['ajaxPageState'])) {
           $post['ajax_page_state[theme]'] = $drupal_settings['ajaxPageState']['theme'];
           $post['ajax_page_state[theme_token]'] = $drupal_settings['ajaxPageState']['theme_token'];
           foreach ($drupal_settings['ajaxPageState']['css'] as $key => $value) {
    -        $post["ajax_page_state[css][$key]"] = 1;
    +        $post["ajax_page_state[css][$key]"] = '1';
           }
           foreach ($drupal_settings['ajaxPageState']['js'] as $key => $value) {
    -        $post["ajax_page_state[js][$key]"] = 1;
    +        $post["ajax_page_state[js][$key]"] = '1';
           }
         }
    

    Bump to » JavaScriptTestBase issue

  14. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2024,12 +2161,13 @@ protected function cronRun() {
    +    // @todo larowlan refactor around Mink
    
    @@ -2052,6 +2190,7 @@ protected function checkForMetaRefresh() {
    +    // @todo larowlan refactor around Mink
    

    @todo can go

  15. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2128,51 +2281,17 @@ protected function handleForm(&$post, &$edit, &$upload, $submit, $form) {
    -              $post[$name] = $value;
    -            }
    -            break;
    -          case 'select':
    -            $new_value = $edit[$name];
    -            $options = $this->getAllOptions($element);
    -            if (is_array($new_value)) {
    -              // Multiple select box.
    -              if (!empty($new_value)) {
    -                $index = 0;
    -                $key = preg_replace('/\[\]$/', '', $name);
    -                foreach ($options as $option) {
    -                  $option_value = (string) $option['value'];
    -                  if (in_array($option_value, $new_value)) {
    -                    $post[$key . '[' . $index++ . ']'] = $option_value;
    -                    $done = TRUE;
    -                    unset($edit[$name]);
    -                  }
    -                }
    -              }
    -              else {
    -                // No options selected: do not include any POST data for the
    -                // element.
    -                $done = TRUE;
    -                unset($edit[$name]);
    -              }
    -            }
    -            else {
    -              // Single select box.
    -              foreach ($options as $option) {
    -                if ($new_value == $option['value']) {
    -                  $post[$name] = $new_value;
    -                  unset($edit[$name]);
    -                  $done = TRUE;
    -                  break;
    -                }
    -              }
    

    goodbye

  16. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2181,27 +2300,11 @@ protected function handleForm(&$post, &$edit, &$upload, $submit, $form) {
    -            $single = empty($element['multiple']);
    -            $first = TRUE;
    -            $index = 0;
    -            $key = preg_replace('/\[\]$/', '', $name);
    -            $options = $this->getAllOptions($element);
    -            foreach ($options as $option) {
    -              // For single select, we load the first option, if there is a
    -              // selected option that will overwrite it later.
    -              if ($option['selected'] || ($first && $single)) {
    -                $first = FALSE;
    -                if ($single) {
    -                  $post[$name] = (string) $option['value'];
    -                }
    -                else {
    -                  $post[$key . '[' . $index++ . ']'] = (string) $option['value'];
    -                }
    -              }
    -            }
    +            $single = !$element->getAttribute('multiple');
    +            $post[$name] = $element->getValue();
    

    mmm cleaner

  17. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2229,6 +2332,70 @@ protected function handleForm(&$post, &$edit, &$upload, $submit, $form) {
    +      $decorated[] = MinkNodeElementDecorator::decorate($element);
    

    We have a decorator for BC, because ::xpath returns SimpleXML element in HEAD, but now returns a Mink NodeElement, this keeps all the old SimpleXML methods/array access valid.

  18. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2320,32 +2492,8 @@ protected function getUrl() {
    -    $request = 0;
    -    $headers = array($request => array());
    -    foreach ($this->headers as $header) {
    -      $header = trim($header);
    -      if ($header === '') {
    -        $request++;
    -      }
    -      else {
    -        if (strpos($header, 'HTTP/') === 0) {
    -          $name = ':status';
    -          $value = $header;
    -        }
    -        else {
    -          list($name, $value) = explode(':', $header, 2);
    -          $name = strtolower($name);
    -        }
    -        if (isset($headers[$request][$name])) {
    -          $headers[$request][$name] .= ',' . trim($value);
    -        }
    -        else {
    -          $headers[$request][$name] = trim($value);
    -        }
    -      }
    -    }
    -    if (!$all_requests) {
    -      $headers = array_pop($headers);
    +    if (!($headers = $this->headers)) {
    +      $headers = $this->getSession()->getResponseHeaders();
    

    much cleaner :)

  19. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2395,7 +2536,9 @@ protected function drupalGetHeader($name, $all_requests = FALSE) {
    -    return $this->getRawContent();
    +    // @todo Refactor to use $this->getSession()->getPage()->getContent() once
    +    //   MinkSession has been sub-classed to allow setting content.
    +    return $this->content;
       }
     
       /**
    @@ -2440,7 +2583,9 @@ protected function drupalGetMails($filter = array()) {
    
    @@ -2440,7 +2583,9 @@ protected function drupalGetMails($filter = array()) {
        * @deprecated 8.x
        *   Use setRawContent().
        */
    -  protected function drupalSetContent($content) {
    +  protected function drupalSetContent($content, $url = 'internal:') {
    +    // @todo Subclass MinkSession to include the ability to setPage() for
    +    //   internal paths.
    

    We're stuck with this, Mink is read-only, lets remove the todo

  20. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2489,6 +2634,345 @@ protected function assertUrl($path, array $options = array(), $message = '', $gr
    +  protected function assertTitle($title, $message = '', $group = 'Other') {
    ...
    +  protected function assertNoTitle($title, $message = '', $group = 'Other') {
    ...
    +  protected function assertFieldByXPath($xpath, $value = NULL, $message = '', $group = 'Other') {
    ...
    +  protected function getSelectedItem($element) {
    ...
    +  protected function assertNoFieldByXPath($xpath, $value = NULL, $message = '', $group = 'Other') {
    ...
    +  protected function assertFieldChecked($id, $message = '', $group = 'Browser') {
    ...
    +  protected function assertNoFieldChecked($id, $message = '', $group = 'Browser') {
    ...
    +  protected function assertOptionSelected($id, $option, $message = '', $group = 'Browser') {
    ...
    +  protected function assertNoOptionSelected($id, $option, $message = '', $group = 'Browser') {
    ...
    +  protected function assertNoDuplicateIds($message = '', $group = 'Other', $ids_to_skip = array()) {
    

    Yep none of these share anything in common with AssertContentTrait so had to be duplicated

  21. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2508,7 +2992,9 @@ protected function assertUrl($path, array $options = array(), $message = '', $gr
    -    $curl_code = curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE);
    +    if (!($curl_code = $this->responseCode)) {
    +      $curl_code = $this->getSession()->getStatusCode();
    +    }
    
    @@ -2533,7 +3019,7 @@ protected function assertResponse($code, $message = '', $group = 'Browser') {
    -    $curl_code = curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE);
    +    $curl_code = $this->getSession()->getStatusCode();
    

    We should remove reference to curl.

  22. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -2694,8 +3180,32 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
    -    $this->container->get('request_stack')->push($request);
    -    $generator->updateFromRequest();
    

    Looks like a merge gone wrong

  23. +++ b/core/modules/simpletest/src/WebTestBase.php
    @@ -29,6 +29,9 @@
    + * @deprecated Deprecated since Drupal 8.x-dev, to be removed in Drupal 8.0.
    + *   Use \Drupal\simpletest\BrowserTestBase for new browser-based tests.
    

    Please Please Please can we?

  24. +++ b/core/modules/system/src/Tests/Form/FormTest.php
    @@ -628,9 +638,9 @@ function testDisabledMarkup() {
    +    $this->forge = array('checkboxes' => array('one' => 'one', 'two' => 'FORGERY'));
    
    @@ -665,4 +675,19 @@ public function testFormStateDatabaseConnection() {
    +  public function onBefore(BeforeEvent $event) {
    

    This is how you monkey the dom - the Mink interface/DomCrawler is read-only, so you can't diddle the dom to e.g. submit option values for select fields that aren't in the dom - so we use a request event and intervene. Note this is Guzzle only.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
136.69 KB
1.17 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,276 pass(es). View

Should fix failing test.

larowlan’s picture

nod_’s picture

epic.

grom358’s picture

FYI I have started a new branch over here https://github.com/grom358/drupal/tree/mink-testing-2232861 where I have made a clean BrowserTestBase. It has no mention of drupalPost or curl stuff etc. It only supports what mink does and in theory should work with any mink driver.

Currently it has a drupalGet method, but thinking it should be renamed to drupalVisit to keep with the mink lingo.

I did some refactoring by pulling up a bunch of non browser methods (ie. drupalCreateRole, etc) from WebTestBase into FunctionalTestBase. And WebTestBase and BrowserTestBase extend this. But down the line would have a HttpTestBase that uses Guzzle with no browser stuff that extends it. With WebTestBase being removed at some point.

One thing have to sort out with HttpTestBase is how to support drupalLogin. The REST tests use this, but currently drupalLogin relies on parsing a form in order to submit the user login form.

moshe weitzman’s picture

drupalLogin should definately not do any http. it should create a session and any other fiddly bits that are needed. that gets stored away for future web requests.

grom358’s picture

FileSize
897.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,359 pass(es), 1 fail(s), and 0 exception(s). View

Patch with BlockHtmlTest ported to the new BrowserTestBase on the branch I outlined before.

grom358’s picture

Status: Needs work » Needs review
FileSize
907.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,896 pass(es). View
dawehner’s picture

It would be great if you guys could provide a patch which does not have the mink code but everything else.

+/**
+ * Test case for typical Drupal tests.
+ *
+ * @ingroup testing
+ */
+abstract class BrowserTestBase extends FunctionalTestBase {

I would be cool to describe that this is for front tests, not API ones.

jibran’s picture

Just for the record you can review/view changes here https://github.com/grom358/drupal/pull/1/files

nick_schuch’s picture

FileSize
1.49 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2232861-28-phpunit-mink.patch. Unable to apply patch. See the log in the details link for more information. View

As per a meeting @larowlan, @grom358 and myself had with @sun, we are working on test class to runs side by side with current simpletest. Between this and @sun's KernelTestBaseNG work well be the lot running on PHPUnit. This is a rough patch on @grom358 and my work.

Here is a demonstration of how to run this work:

$ DOMAIN=d8-phpunit.dev core/vendor/bin/phpunit -c core --testsuite functional --group modern --debug --testdox

PHPUnit 4.1.3 by Sebastian Bergmann.

Configuration read from /Users/nickschuch/projects/personal/phpunit-d8/app/core/phpunit.xml.dist

Drupal\simpletest\Tests\BrowserTestBase
 [x] Go to

Still do to

Currently we need to add more test coverage.

grom358’s picture

FileSize
1.56 MB

Some cleanup and polish.

grom358’s picture

Status: Needs work » Needs review
FileSize
1.56 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Small fix

grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,038 pass(es), 2 fail(s), and 0 exception(s). View
nick_schuch’s picture

If anyone is willing to sprint on this issue, we are looking for someone to write the test suite that ensures that this new functionality is working. We have a class for these tests (https://github.com/nickschuch/drupal-phpunit-mink/blob/phpunit-mink/core...) and a module (https://github.com/nickschuch/drupal-phpunit-mink/tree/phpunit-mink/core...).

The kinds of test we are looking for are:

  • Viewing a page and asserting text.
  • Submitting forms.
  • Writing a second class to ensure we don't have bleed through between tests (sessions).
moshe weitzman’s picture

Just to follow-up, we sprinted on this at Badcamp. Half a day was lost to getting it to work. Thanks to @grom for waking up and helping get the right webAssert class fired up for submitting our forms.

We ended up submitting a PR at https://github.com/nickschuch/drupal-phpunit-mink/pull/5.

I'm not sure that a second class that proves or disproves session bleed is possible. The second class needs to depend on the first and thats not really what the @depends annotation means in phpunit.

nick_schuch’s picture

I just mean we write 2 separate test case classes to ensure that we don't have the same mink session between one test class and another.

nick_schuch’s picture

@grom, looks like it's not the port, it's more to do with that domain variable not being set in BrowserTestBase.

I got the run-tests.sh command from the bot output.

/usr/bin/php ./core/scripts/run-tests.sh --concurrency 25 --php /usr/bin/php --url http://ec2-54-179-121-168.ap-southeast-1.compute.amazonaws.com/checkout --keep-results --all --clean 2>&1

Can be found here: https://qa.drupal.org/pifr/test/902993

grom358’s picture

Status: Needs work » Needs review
FileSize
1.03 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,153 pass(es), 4 fail(s), and 0 exception(s). View
grom358’s picture

Status: Needs work » Needs review
FileSize
1.03 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,196 pass(es), 2 fail(s), and 0 exception(s). View
grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 48,672 pass(es), 8,963 fail(s), and 7,081 exception(s). View
grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,198 pass(es), 1 fail(s), and 0 exception(s). View
grom358’s picture

FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,155 pass(es), 1 fail(s), and 0 exception(s). View
grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,152 pass(es). View
grom358’s picture

So it runs on testbot via run-tests.sh now :)

But for those running directly with phpunit, please note I changed it to use environment variable SIMPLETEST_BASE_URL and its a url to base url for your site and you can run directly from inside the core directory too. Eg:

grom358 /vagrant/app/core $ SIMPLETEST_BASE_URL=http://d8.dev:8080/drupal ./vendor/bin/phpunit --testsuite functional --group modern --debug

Test suite still needs to test more of BrowserTestBase, eg. logging in (drupalLogin) etc.

phenaproxima’s picture

@moshe told me he would take that on (the login stuff, adding more advanced self-test cases).

jibran’s picture

Here is something reviewable without vendor libraries.

jibran’s picture

Yay!!! it is green now. Some minor stuff.

  1. +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.info.yml
    @@ -0,0 +1,6 @@
    +name: Testing test
    

    Tests simpletest module.

  2. +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.info.yml
    @@ -0,0 +1,6 @@
    +description: 'Provides dummy classes for use by SimpleTest tests.'
    

    I think we can make this better.

  3. +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.info.yml
    @@ -0,0 +1,6 @@
    diff --git a/core/modules/simpletest/tests/simpletest_test/simpletest_test.module b/core/modules/simpletest/tests/simpletest_test/simpletest_test.module
    
    diff --git a/core/modules/simpletest/tests/simpletest_test/simpletest_test.module b/core/modules/simpletest/tests/simpletest_test/simpletest_test.module
    new file mode 100644
    
    new file mode 100644
    index 0000000..651884e
    
    index 0000000..651884e
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.module
    
    +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.module
    +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.module
    @@ -0,0 +1,6 @@
    
    @@ -0,0 +1,6 @@
    +<?php
    +
    +/**
    + * @file
    + * Provides tests for simpletest_test module
    + */
    

    Not needed now.

  4. +++ b/core/modules/simpletest/tests/simpletest_test/src/Controller/SimpletestController.php
    @@ -0,0 +1,29 @@
    +      '#markup' => $this->t('Hello Amsterdam'),
    

    Hello Drupal :D

  5. +++ b/core/modules/simpletest/tests/simpletest_test/src/Form/ExampleForm.php
    @@ -0,0 +1,46 @@
    +    // Normally this config object would be injected but this is a only a test
    +    // so...who cares?
    

    LOL

  6. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
    @@ -0,0 +1,61 @@
    + * Definition of Drupal\Tests\simpletest\Functional\BrowserTestBaseTest.
    

    Contains \Drupal\Tests\simpletest\Functional\BrowserTestBaseTest.

larowlan’s picture

Issue summary: View changes

Updating commit mentions in issue summary

larowlan’s picture

Issue summary: View changes
phenaproxima’s picture

@jibran, glad you enjoyed my editorializing in #5 ;-)

moshe weitzman’s picture

FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,961 pass(es). View
2.43 KB

This followup verifies that drupalLogin() is working in a BrowserTestBase case. BrowserTestBase is now well tested enough for initial commit, IMO.

nick_schuch’s picture

Myself, @larowan, @grom358 and @phenaproxima had a call a couple of days ago with the goal of putting together a list of things we would like to see on a "Simpletest roadmap". These are the notes, @moshe is there anything you would like to add? If all good I would like to formalise these in a separate ticket / groups.drupal.org post.

----------

Simpletest - Roadmap
https://www.drupal.org/node/2304461 - kerneltestbaseng to make kernel test base run on phpunit
Step 1: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/DrupalKernel...
No seriously. The kernel needs to be dependency injected before the rest happens. :-)
https://drupal.org/node/2232861 - mink + phpunit to replace webtestbase

follow ups for phpunit+mink

  • Create a meta an spin off sub-issues for each module in core
  • Address which methods in BrowserTestBase don't have coverage in BrowserTestBaseTest and file issues to expand them
  • Start on a PhantomJs Driver
  • Start on BrowserKitDriver - exit() calls are gone from FormBuilder now so the bulk of exit() calls left in core are exception paths, ie screwy stuff in the installer - most of the tests shouldn't hit these so we should be able to use a browser kit
  • Make an issue to rip out the run-tests.sh wraps phpunit nonsense - rely on phpunit runner for real. But we need Drupal CI first.
  • Implement PHPUnit concurrency contrib project - https://packagist.org/packages/verkkokauppacom/parallel-phpunit
  • Swap out Simpletest UI with PHPUnit contrib - https://github.com/NSinopoli/VisualPHPUnit
  • Rename “Simpletest” to “Testing”

To do on kerneltestbaseng

  • split into smaller chunks -
  • Port away from WebUnitTestBase
  • vfs stuff
  • random trait
  • assert legacy trait?
  • sqllite stuff
grom358’s picture

FileSize
1.03 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
grom358’s picture

FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
grom358’s picture

Status: Needs work » Needs review
grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,767 pass(es). View
dawehner’s picture

It's a bit odd that we point to goutte driver master ... don't we have a proper stable release for it?

# BrowserTestBase

+  /**
+   * The DrupalKernel instance used in the test.
+   *
+   * @var \Drupal\Core\DrupalKernel
+   */
+  protected $kernel;

Just be clear, we have an interface for that.

daffie’s picture

My first observations:

+++ b/core/includes/bootstrap.inc
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+if (!defined('REQUEST_TIME')) {
+  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+}

-define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+if (!defined('DRUPAL_ROOT')) {
+  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+}

Why do you change that?

+++ b/core/scripts/run-tests.sh
+      // Process phpunit web-tests next as they are faster.
+      if (is_subclass_of($test_class, 'Drupal\simpletest\BrowserTestBase')) {
+        simpletest_script_run_phpunit($test_id, $test_class);
+        continue;
+      }

Can we rewrite that as

+      if (is_subclass_of($test_class, '\PHPUnit_Framework_TestCase') ||
+          is_subclass_of($test_class, 'Drupal\simpletest\BrowserTestBase')) {
+        simpletest_script_run_phpunit($test_id, $test_class);
+        continue;
+      }
+++ b/core/modules/simpletest/src/BrowserTestBase.php
+  /**
+   * {@inheritdoc}
+   */
+  public function installDrupal() {

Where is the documentation inherit from?

I have a few concerns about the naming of files. We have an example module simpletest_test with a form ExampleForm. We also have the simpletest module with the form SimpletestTestForm. For me it is a bit confusing. Can we rename the simpletest_test module to simpletest_example. Can we also rename BrowserTestBaseTest to SimpletestExampleTest. So that for developers that are new to our PHPUnit-behat-mink testing, they can easily find the example module and testfile.

daffie’s picture

Status: Needs review » Needs work

Overall a great patch! I like it very much.

daffie’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
+  public function assertSession($name = NULL) {
+    return new WebAssert($this->getSession($name));
+  }

There is no use Drupal\Simpletest\WebAssert;

larowlan’s picture

Status: Needs work » Needs review
FileSize
75.49 KB
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,835 pass(es). View

#71 - same namespace - so no need for use stmt

re-roll and also a core-only patch.

jibran’s picture

It is sad that there is no fix for #56 yet :( here is another review.

  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    + * Definition of \Drupal\simpletest\BrowserTestBase.
    

    Contains \Drupal\simpletest\BrowserTestBase.

  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * @var object
    

    Do we have a type?

  3. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * Time limit for the test.
    

    Is it in seconds? Please update the doc block.

  4. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * This is set in prepareEnvironment().
    ...
    +   * This is set in prepareEnvironment().
    ...
    +   * This is set in prepareEnvironment().
    ...
    +   * This is set in prepareEnvironment().
    

    I think this should be self::prepareEnvironment().

  5. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * The current session name, if available.
    

    @var line missing.

  6. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * @var UserSession
    ...
    +   * @var Mink
    

    Full namespace please.

  7. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * Create a user with a given set of permissions.
    

    Creates

  8. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +    $account = entity_create('user', $edit);
    ...
    +    $role = entity_create('user_role', array(
    ...
    +        $assigned_permissions = entity_load('user_role', $role->id())->getPermissions();
    

    Can we use static create/load methods here?

  9. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * Log in a user with the internal browser.
    

    Logs

  10. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +    // @see WebTestBase::drupalUserIsLoggedIn()
    

    Full namespace please.

  11. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +   * Fill and submit a form.
    

    Fills

  12. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1359 @@
    +        error_log('8');
    

    Please add comment for this.

  13. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -0,0 +1,70 @@
    +class WebAssert extends \Behat\Mink\WebAssert {
    

    Can we add a use statement on top and call it MinkWebAssert?

  14. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -0,0 +1,70 @@
    +   * Checks that specific field exists on the current page.
    

    not field button.

  15. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -0,0 +1,70 @@
    +   * Checks that specific field exists on the current page.
    

    not field select.

  16. +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.info.yml
    --- /dev/null
    +++ b/core/modules/simpletest/tests/simpletest_test/simpletest_test.module
    

    We don't need this file anymore.

daffie’s picture

Status: Needs review » Needs work

My points from comment #69 are still open.

grom358’s picture

Status: Needs work » Needs review
grom358’s picture

FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,817 pass(es). View
grom358’s picture

@dawehner comment #68 . Rely on DrupalKernel::rebuildContainer() so currently can not use the interface, since it doesn't define that as part of interface. Suggested solutions?

@daffie comment #69 I believe those ifs are due to getting redefined DRUPAL_ROOT constants when phpunit does it test discovery. Have to check with nick_schuch on that one since he was one that dealt with getting this to work with phpunit.

About comment #71 . There a \Drupal\simpletest\WebAssert that extends the \Behat\Mink\WebAssert and adds some additional methods. Should try get this added upstream.

As for proposed changes @daffie and @jibran I just uploaded another patch updated according to your reviews.

daffie’s picture

@grom358: Can you add an interdiff for the drupal files.

grom358’s picture

FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,828 pass(es). View
grom358’s picture

FileSize
14.53 KB
jibran’s picture

Core only patch

amateescu’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -0,0 +1,1361 @@
+  protected function drupalUserIsLoggedIn($account) {
+    if (!isset($account->session_id)) {
+      return FALSE;
+    }
+    // The session ID is hashed before being stored in the database.
+    // @see \Drupal\Core\Session\SessionHandler::read()
+    return (bool) db_query("SELECT sid FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid", array(':sid' => Crypt::hashBase64($account->session_id)))->fetchField();
+  }

I'm not sure how well tested this new BrowserTestBase is, but at least this method is clearly broken since 'sid' is not stored in {users} anymore for quite a few months.

Instead of copy-pasting methods from WebTestBase and having to maintain both in the future, wouldn't it be better to extract the similarities in a trait?

grom358’s picture

@amateescu yeah some of these (ie. the random methods) could be moved to a trait. Though the idea is for BrowserTestBase to replace WebTestBase so we don't have to maintain so much code for a test framework. Also we do need more test coverage. Having said that... drupalLogin calls

$this->assertTrue($this->drupalUserIsLoggedIn($account), String::format('User %name successfully logged in.', array('name' => $account->getUsername())));

So mmmmm. Get back to you on that one.

grom358’s picture

FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.a0add69.patch. Unable to apply patch. See the log in the details link for more information. View
2.98 KB
75.12 KB

Updated drupalUserIsLoggedIn and fix couple other minor things.

grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.b830771.patch. Unable to apply patch. See the log in the details link for more information. View
75.12 KB

Patch reroll

grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.e387bd4.patch. Unable to apply patch. See the log in the details link for more information. View

patch reroll

daffie’s picture

@grom358: Can you add interdiff.txt to your patch. It will be easier for others to follow what your changes are.

benjy’s picture

Any specific reason BrowserTestBase has a handful of class properties using camel case and then others with lowercase and underscores?

grom358’s picture

@benjy no... leftover cruff from WebTestBase . Will fix.

@daffie patch reroll so there is no interdiff if I'm not mistaken. I am kind of noob to the whole interdiff thing still.

daffie’s picture

@grom358: A tutorial on how to make an interdiff.txt.

daffie queued 89: phpunit-mink.e387bd4.patch for re-testing.

daffie’s picture

Issue tags: +Needs reroll
grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,557 pass(es). View
8.38 KB

Reroll and changed to lowerCamel property names

grom358’s picture

Patch without vendor for review purpose

benjy’s picture

I had a quick read over the code, mainly just syntax based stuff below, I realise much of the code came across from WebTestBase soI apologies for anything that might not have been new. I'll try have a full test of this over the weekend. Look good!

  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +  protected $siteDirectory = NULL;
    ...
    +  protected $databasePrefix = NULL;
    

    These don't need to be initialised to NULL, and you don't do it for other properties on the same class.

  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +   * @var Mink
    

    Absolute class name.

  3. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +  public function __construct($test_id = NULL) {
    +    parent::__construct($test_id);
    +  }
    

    Is this needed? Do we use the, don't pass an Id use case and therefore default to NULL use case?

  4. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +    if ($parsed_url['scheme'] == 'https') {
    

    No reason not to use a strict check here.

  5. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +    $parts = parse_url($path);
    +    if (empty($parts['host'])) {
    +      // Ensure that we have a string (and no xpath object).
    +      $path = (string) $path;
    

    So parse_url() successfully handles the xpath objects? It would seem to me that we should cast it first?

  6. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +    else {
    +      return FALSE;
    +    }
    

    Don't actually need the else block here.

  7. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +   * @throws \Exception When exception was thrown inside the test.
    

    Description should be on the next line.

  8. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +    // system_requirements() removes write permissions from settings.php
    +    // whenever it is invoked.
    +    // Not using File API; a potential error must trigger a PHP warning.
    

    Does this need a separate issue to discuss?

  9. I also noticed in quite a few places you're missing (optional) for parameters as per: https://www.drupal.org/coding-standards/docs#param
daffie’s picture

Status: Needs review » Needs work

I would like to help to get this patch committed. In the remaining tasks you state that you want to "Write a test suite to test itself". If I then look at the code I come to the conclusion that that will become very difficult. My idea would be to start writing tests and to see what the problems are and fix those. After we write some non trivial tests and we get the results that we want we can say that this patch is RTBC. I have selected the tests in core/modules/simpletest/src/Tests for this. The first big problem I encountered was that the class Drupal\simpletest\WebAssert needs a lot more methods for all kinds of asserts. The second problem is that we use the GoutteDriver and that does not support javascript.
If anyone has other/better ideas please lets hear them.

grom358’s picture

@daffie the test suite testing itself is BrowserTestBaseTest. It covers a basic page controller and form at the moment. Basically its an example on how to use BrowserTestBase. Ideally I would like to port tests using WebTestBase to BrowserTestBase.

Can you expand on what methods for all kinds of asserts? That class extends the WebAssert from Mink and has all sorts of asserts. Ideally I would want Drupal\simpletest\WebAssert to disappear and have those additional methods go upstream into Mink.

As for support of javascript... there are some issues that need resolving for that. The current testbot infrastructure to my knowledge can't support adding in something like phantomJS. So that means stuck with GoutteDriver for now.

It would be nice to be able to register sessions with BrowserTestBase via YML config. Then in the test when need javascript support can do something like..

$this->setDefaultSession('phantomJS');

and then all the mink commands/asserts will be using the Javascript driver.

pfrenssen’s picture

I think providing a driver that supports javascript is out of scope for this issue. Once this is in It will be easy enough to override this class and use PhantomJS / Selenium / ZombieJS. We can always add support for this in a future issue.

The same goes for adding assert methods, or a non-trivial test case. Those things are out of scope for this issue. This issue is just about getting a basic browser test framework in which is incredibly valuable in itself. We can always add followups to port existing non-trivial tests over and the need for additional assert methods will become apparent then. I actually doubt whether we need much more than what Mink offers out of the box in the base class.

Going to have a look at the patch now during my lunch break.

  1. +++ b/composer.json
    @@ -5,6 +5,9 @@
    +    "behat/mink-goutte-driver": "dev-master",
    +    "fabpot/goutte": "dev-master",
    

    Don't rely on branch heads, since these might contain major changes at some point in the future. It's better to specify version numbers or tags.

    If you do need to rely on the master branch because you need some unreleased features / fixes, then "pin" the most recent commit ref (eg. "dev-master#a23adb345").

  2. +++ b/core/includes/bootstrap.inc
    @@ -113,7 +113,9 @@
    -define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
    +if (!defined('REQUEST_TIME')) {
    +  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
    +}
    

    Is this needed to avoid notices being thrown when Drupal is bootstrapped twice during a browser test run? It might be a good idea to add a comment here explaining why these constants may already be defined.

  3. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +use Behat\Mink\Exception\Exception;
    +use Behat\Mink\Mink;
    +use Behat\Mink\Session;
    +use Behat\Mink\Element\Element;
    +use Behat\Mink\Element\NodeElement;
    +use Behat\Mink\Driver\GoutteDriver;
    

    Maintain alphabetical ordering.

  4. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +  protected $originalSite;
    

    I would rename this property to $originalSiteDirectory to better convey its contents and be consistent with $siteDirectory.

  5. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1362 @@
    +  /**
    +   * @var Mink
    +   */
    +  protected $mink;
    

    I know it's glaringly obvious what this contains, but we need a description for this ;)

I got only half way through the patch, my lunch break is over :)

grom358’s picture

Status: Needs work » Needs review
FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,465 pass(es). View
74.33 KB
12.1 KB

Reroll and code cleanup based on suggestions from @benjy and @pfrenssen

I agree with what pfrenssen stated. This issue is just concerned with getting BrowserTestBase into core. The other things I mention are other issues to go into a meta issue about deprecating simpletest.

larowlan’s picture

Can we add a deprecated tag to WebTestBase here? Earlier patches had it.

hussainweb’s picture

Issue tags: -Needs reroll
FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,872 pass(es). View
74.37 KB

Rerolling... I worked from the -do-not-test.patch in #103 and after rerolling, ran composer install and created the patch. I have also attached a similar do-not-test.patch.

daffie’s picture

@hussainweb: Can you add an interdiff.txt. I think that it is best to use the do-not-test patches for that.

hussainweb’s picture

@daffie: It is complicated (and sometimes meaningless) to get interdiffs for rerolls. That is the reason I usually don't change anything in a reroll. For changes and additions, I will definitely use an interdiff.

daffie’s picture

@hussainweb: Thank you for telling that you did a straight reroll and no other changes or additions!

daffie’s picture

FileSize
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,901 pass(es). View
69.48 KB

I removed the test module simpletest_example. I did this because there are other test modules in the system module that we can use for this patch. I have chosen test_page_test and form_test. I have changed the test BworserTestBaseTest a bit so that it uses the new test modules. If you think that this is wrong please let me know.

grom358’s picture

@daffie do you have an interdiff so I can apply to repo? also are you on github. Could add you to the github repo working on this issue with

hussainweb’s picture

FileSize
7.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-105-109.patch. Unable to apply patch. See the log in the details link for more information. View

I'm attaching the interdiff between #105 and #109.

hussainweb’s picture

Sorry, I didn't mean to have that go to test. I saved it as .patch by mistake.

grom358’s picture

Status: Needs work » Needs review
FileSize
7.2 KB

@hussainweb can also add you to the github repo.. just need to know github username

hussainweb’s picture

It's hussainweb. Thanks!

daffie’s picture

@grom538: My github account is daffie1.

larowlan’s picture

Issue tags: +Needs reroll
pcambra’s picture

Issue tags: -Needs reroll
FileSize
1.02 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,140 pass(es), 4 fail(s), and 0 exception(s). View

Plain reroll of this, only the composer.lock file had conflicts

pcambra’s picture

I don't seem to reproduce the fails of the testbot locally (it fails for me on t() being redeclared) but they seem related to https://www.drupal.org/node/2407153

benjy’s picture

The BrowserTestBaseTest errors are likely this:

+    $config->get('system.file')
+      ->set('path.private', $this->privateFilesDirectory)
+      ->set('path.temporary', $this->tempFilesDirectory)
+      ->save();

$config->getEditable('system.file') should fix that.

EDIT: I didn't test that, i just had a similar issue elsewhere earlier.

pcambra’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
1.02 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,129 pass(es). View

Using getEditable from the config factory as @benjy suggests.

grom358’s picture

@daffie and @hussainweb have add you to github repo https://github.com/nickschuch/drupal-phpunit-mink/

Happy to add anyone else that wishes to contribute via github, having said that I will still gladly take interdiffs/patches.

grom358’s picture

FileSize
78.03 KB
1.69 KB
1.9 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,464 pass(es). View

@pcambra you attached the wrong interdiff there it seems. I believe I have made the same changes however on the github repo.

benjy’s picture

ha, #122 is my interdiff from another issue :)

@grom358, where are you at with this issue, are you simply waiting on reviews at this point?

grom358’s picture

@benjy yes just waiting on reviews.

As far as I am concerned the functionality meets the goal of this particular issue. That is a test base that uses phpunit + mink. There are further goals which are related to this issue but this issue is step 1. One of which is to support other drivers and hence javascript. Longer term the goal is to move away from having to maintain the custom testing framework that is simpletest.

pcambra’s picture

@pcambra you attached the wrong interdiff there it seems. I believe I have made the same changes however on the github repo.

I don't think so... unless you guys see another file there: https://www.drupal.org/files/issues/interdiff_9537.txt

Patch in #124 is 1.9M and the original is 1.02M but the interdiff is 1.69Kb, there's something happening there.

benjy’s picture

I don't think so... unless you guys see another file there: https://www.drupal.org/files/issues/interdiff_9537.txt

Yeah that was showing me the wrong file, I added a ?t=1 on the end and get the right version. Guess it was cached.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Time to get this in, and start practising the dance we'll perform on simpletest's grave?

grom358’s picture

@pcambra the increase in file size is most likely from reroll and changing of composer.json. Previously I was using dev-master and I changed to use stable releases. I can't see anything else in the patch to explain the change in filesize. The do-not-test patch has not varied in size.

I am looking into it as double check.

benjy’s picture

+++ b/core/includes/bootstrap.inc
@@ -113,7 +113,9 @@
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+if (!defined('REQUEST_TIME')) {
+  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+}

@@ -141,7 +143,9 @@
-define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+if (!defined('DRUPAL_ROOT')) {
+  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+}

This code has been mentioned twice in this issue but I can't see a response.

Can we add a comment as to why this is now needed. I don't see why bootstrap.inc is included twice so i'm guessing something else has already defined them?

alexpott’s picture

Assigned: larowlan » Unassigned
Status: Reviewed & tested by the community » Needs work

Adding new libraries is a Dries decision. So please assign to Dries upon re-rtbc.

Also it looks like the issue summary is in need of an update/rewrite since I'm not sure that the API changes listed are happening. PLus the suggested commit message is out of date - however since everyone has contributed a file to the issue I suggest removing it since the auto-generated one looks good.

benjy’s picture

FileSize
3.56 KB

Spent an hour testing this today. I think the issue summary needs some instructions to make this easier for people to test. I noticed that tests running BrowserTestBase cannot be run from the UI or run-tests.sh, was that intended?

  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1342 @@
    +  public function setUp() {
    

    This should be protected inline with how WebTestBase was previously.

  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1342 @@
    +  public function tearDown() {
    

    Should be protected like WebTestBase.

  3.   protected function config($name) {
        return \Drupal::configFactory()->getEditable($name);
      }
    

    Maybe we could copy this out of TestBase into BrowserTestBase? Many tests likely use this function and inherit it through WebTestBase? Also configImporter(), copyConfig().

  4. Most tests now use SchemaCheckTestTrait, which calls $this->pass and that no longer exists on BrowserTestBase. Not sure how we convert tests that use this trait?

Interdiff attached shows the changes I had to make to get MigrateAggregatorConfigsTest working, pretty easy.

grom358’s picture

Issue summary: View changes
grom358’s picture

@benjy can you provide more details on using run-tests.sh and simpletest UI. That is definitely something that should be working, they are working for me here locally and parts of the patch was to make it work with them. You can see the test bots runs BrowserTestBaseTest which is using run-tests.sh

benjy’s picture

1. Simpletest UI - This is still broken for me. I had the patch #124 applied on top of 1e985b0 plus the interdiff from #133 and tried to run MigrateAggregatorConfigTest which failed with Drupal\Core\Installer\Exception\AlreadyInstalledException: (screenshot attached).

2. run-tests.sh - Yes this is working for me now since my last re-install, not sure what was going on before.

I also tried without my interdiff using BrowserTestBaseTest for both run-tests and UI. Same result, UI fails, run-tests works.

phenaproxima’s picture

FileSize
78.03 KB
1.9 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch phpunit-mink.9cdc0fe.patch. Unable to apply patch. See the log in the details link for more information. View

Patch rerolled with invaluable assistance from @grom358.

grom358’s picture

Status: Needs work » Needs review

Setting status to needs review so bot tests rerolled patch.

daffie’s picture

Issue tags: -JavaScript +9/JavaScript, +Needs reroll
daffie’s picture

Issue tags: -9/JavaScript +JavaScript
hussainweb’s picture

Issue tags: -Needs reroll
FileSize
78.03 KB
1.9 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,722 pass(es). View
hussainweb’s picture

Status: Needs work » Needs review
benjy’s picture

Status: Needs review » Needs work

Test request has been sent for #143, back to NW for #136

Status: Needs work » Needs review
grom358’s picture

@benjy RE #136 what error do you get for BrowserTestBaseTest with SimpleTest UI? Haven't been able to get hold of you on IRC. What is your environment setup? Considering the UI and run-tests.sh call the same code for running the test, I thinking could be something around SIMPLETEST_BASEURL environment variable and so configuration doesn't come through correctly.

Porting of existing tests such as MigrateAggregatorConfigTest I want to make out of scope of this particular ticket. Once this test base is in, I was going to make a Meta and go from there so we can start phasing out SimpleTest. Having said that though, perhaps there is a bug with BrowserTestBase. BrowserTestBaseTest may need more test cases if so.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

It all looks good to me.
The problem of @benjy that he cannot run his own migrated tests is something for a follow-up issue.
The patch adds the basic functionality to do functional testing. There is a basic test in the patch to prove that it works.
The patch adds a two new libraries to core: behat/mink and behat/mink-goutte-driver. These need a @Dries approval.
All documentation is in order.
It gets a RTBC from me.

alexpott’s picture

Assigned: Unassigned » Dries

New libraries are for Dries to approve.

I'm extremely ++ to this change.

benjy’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Needs work

@daffie, no the screenshot showed the migrate test, the exact same problem also occurs with BrowserTestBaseTest as I mentioned in #136. (http://i.imgur.com/0yqzOXB.png)

Also, the issues from #133 still haven't been fixed either.

daffie’s picture

FileSize
74.02 KB

@benjy: Running the BrowserTestBaseTest is working for me. With run-tests.sh an with the SimpleTest UI. Did you try the suggestion of @grom358?

For #133.3 and #133.4: I think that those should be addressed in followups.

daffie’s picture

Status: Needs work » Needs review
FileSize
78.03 KB
1.9 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,771 pass(es). View
562 bytes

Applied the requested changes from #133.1 and #133.2. The functions BrowserTestBaseTest::setup and BrowserTestBaseTest::teardown are now protected.

benjy’s picture

@daffie, awesome thanks for making those changes.

@grom358 didn't offer any possible solutions only a suggestion that it might be SIMPLETEST_BASEURL. We discussed this in IRC earlier today, it dies early in the installation process, I said I would try to debug this further. I use the UI for running tests every day and i've never had the install fail like this with WebTestBase.

I tested this in a container and the test passes, the problem is when you have Drupal installed in a sub directory that the tests fail from the UI.

daffie’s picture

@benjy: Do you have any objections on putting this issue back to RTBC?

ParisLiakos’s picture

the problem is when you have Drupal installed in a sub directory that the tests fail from the UI.

I have it installed in a subdirectory but the test passes for me. Probably the problem is too localized to hold up this issue

alexpott’s picture

So this works for me in a sub directory - through the UI, run-tests, and phpUnit.

Obviously we have a massive regression wrt to the output that is available to the user through both phpUnit and run-tests / Simpletest UI. We now longer see the pages if verbose is on etc. However trying to implement all of that is step 1 is unrealistic and will hinder rather than help effort. What this does mean is that no Simpletests should be converted until we have a way to achieve feature parity (and hopefully enhancements).

alexpott’s picture

Before assigning this issue to Dries again it'd be great it someone can update the issue summary to reflect the patch and sell the vision.

lokapujya’s picture

I had to hardcode the SIMPLETEST_BASEURL and run phpunit from d8/core and as an admin(well I added _www to the admin group) to get phpunit to work from command line. So, I tried hardcoding the $phpunit_bin to a relative path like this: "./vendor/bin/phpunit" (since it works like that from command line) and I still get the same error as benjy running from UI.

larowlan’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Added some sizzle to the issue summary

larowlan’s picture

Assigned: Unassigned » Dries

As per #157

daffie’s picture

Status: Needs review » Reviewed & tested by the community

It all looks good to me.
The patch adds the basic functionality to do functional testing. There is a basic test in the patch to prove that it works.
The patch adds a two new libraries to core: behat/mink and behat/mink-goutte-driver. These need a @Dries approval.
All documentation is in order.
I know that you are not allowed to give your own patch a RTBC, but my change is so minor that I think that it is allowed. So it gets a RTBC from me.

benjy’s picture

What this does mean is that no Simpletests should be converted until we have a way to achieve feature parity (and hopefully enhancements).

In that case, +1 for RTBC.

alexpott’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
    @@ -0,0 +1,62 @@
    +namespace Drupal\Tests\simpletest\Functional;
    

    We have a problem with using Drupal\Tests\ as a namespace start. At the moment this is used in TestDiscovery::getTestInfo to assign the group if it is a PHPUnit test. Whilst in some ways this is not wrong, it is a PHPUnit test, I don't think it is correct.

    I think we should add a new namespace to help us determine what type the test is. We can not use reflection here because we have too many tests - a good problem to have. See #2422019: Don't use reflection for parsing test annotations for more.

  2. What this does mean is that no Simpletests should be converted until we have a way to achieve feature parity (and hopefully enhancements).

    Can we get a code comment about the fact that WebTestBase tests are still the way to do functional testing that requires a browser as we've not achieved feature parity yet.

grom358’s picture

@lokapujya and @benjy I would like to fix this issue having... unfortunately I haven't been able to replicate it yet. Until then its very hard for me to fix the issue.

If you could debug and find the point it breaks that maybe might make me dig into the code at the point on possible causes.

@alexpott in addition to no verbose output (which I already have ideas on how to add), the other issue is Goutte driver also doesn't do AJAX requests where Simpletest has some implementation that mimics what the javascript code does. This is just a driver issue though and be solved by new Drupal CI being worked on.

daffie’s picture

Status: Needs work » Needs review

For #163.1: All tests in the namespace "Drupal\Tests\" are now grouped together in the group "PHPUnit". I think that that grouping is wrong. The more PHPUnit tests are added the bigger that group will get. Tests need to be grouped for what they tests and not how they are tested. A PHPUnit test that tests some block functionality needs to be grouped in the group "block" and not in "PHPUnit".
With that in mind I think that the problem is in the grouping and not the namespace that this patch is using.

User @grom358 addressed #163.2. So for me the issue can go back to RTBC.

@alexpott do you have any objections for setting this issue back to RTBC?

tstoeckler’s picture

The subdirectories between $modulename/tests/src where we currently only have Unit where originally envisioned to become separate PHPUnit test suites. Regardless, I think - in lou of a real Simpletest UI revamp - it would at least make sense to make the "test suite name" part of the group as it shows up in the Simpletest UI. I.e. what is now PHPUnit would become PHPUnit: Unit tests and then with this patch we would also have PHPUnit: Functional tests. Or maybe just Unit and Functional, but you get the idea. I think that would solve @alexpott's concerns and also be fairly easy to implement without postponing this further on improvements to the Simpletest UI.

daffie’s picture

alexpott’s picture

alexpott’s picture

The groups not only affect the simpletest UI but also run-tests.sh

tstoeckler’s picture

Re #169: Right, good point!

daffie’s picture

The groups not only affect the simpletest UI but also run-tests.sh

Can someone explain why that is important? With run-tests.sh you can also test as a group. The same as with the simpletest UI.

lokapujya’s picture

I think I discovered the cause the webUI problem. phpunit was working from run-tests.sh, but not from webUI. Running on a mac. The tests are both run as the same user so you would think that if CL works that webUI should work.

In addition to the other things in #158, I hardcoded an export path command into the exec() before calling phpunit so that the webUI uses the same path as the CL. Now my webUI tests seem to be working.

So in simpletest_phpunit_run_command(), I had to do this:

$setpath = 'export PATH=[MY PATH FROM COMMAND LINE]';
$ret = exec($setpath.$the_command, $output);

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/phpunit.xml.dist
@@ -6,9 +6,10 @@
       <directory>./modules/*/tests</directory>

@@ -20,6 +21,15 @@
+      <directory>./modules/*/tests/src/Functional</directory>

Only modules can have functional tests? Also won't the unit test suite also contain the functional test suite?

daffie’s picture

Talked to @dawehner on IRC. He wanted the private functions BrowserTestBase::changeDatabasePrefix() and BrowserTestBase::prepareDatabasePrefix() made protected.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.17 KB
83.35 KB
1.9 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,916 pass(es), 1 fail(s), and 0 exception(s). View
  • Ensured that functional tests end up in their @group.
  • Added a test to ensure that the functional test can be run through the UI.
  • Made the Unit and Functional namespace explicit for modules in TestDiscovery::registerTestNamespaces().
  • Added the ability for core to have functional tests.
  • Fixed #173

It is a shame that ./core/tests/Drupal/Tests is not ./core/tests/Drupal/Unit or ./core/tests/Drupal/Tests/Unit but changing that now would be painful and very disruptive.

[Edit: completed sentence]

daffie’s picture

Looks good to me.

  1. +++ b/core/phpunit.xml.dist
    @@ -10,10 +10,10 @@
    +      <directory>../modules/*/tests/src/Unit</directory>
    +      <directory>../sites/*/modules/*/tests/src/Unit</directory>
    

    Do we need these two lines in <testsuite name="functional">. With Functional instead of Unit?

  2. The PHPUnit tests are grouped together. For Functional PHPUnit tests is that not the case. Just to be sure: Is this how you want it @alexpott.
  3. Comment #174 still needs to be addressed.
alexpott’s picture

re #177

  1. Yes we do. Nice catch
  2. Yes, functional tests should behave exactly the same way as Simpletest webtests.

That fail is real shame - it works locally for me. How to debug that... not sure atm.

benjy’s picture

While there is some re-rolling going on, i've noticed it mentioned three times in this issue but no answer or response from what I can tell:

+++ b/core/includes/bootstrap.inc
@@ -113,7 +113,9 @@
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+if (!defined('REQUEST_TIME')) {
+  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+}
@@ -141,7 +143,9 @@
-define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+if (!defined('DRUPAL_ROOT')) {
+  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+}

Why do we need these?

grom358’s picture

@benjy you are correct. That question has not been addressed yet.

larowlan’s picture

The reasoning was that phpunit loads the actual files during discovery and hence the unit test suite has some instances where these are (hackily) declared to ensure they pass.

When we get to functional tests we bootstrap drupal, and hence some of the earlier unit tests might have already declared these.

It might not be a problem any more with separate test suites.

daffie’s picture

Whats needs to be done for this issue

  1. The two functions BrowserTestBase::changeDatabasePrefix() and BrowserTestBase::prepareDatabasePrefix() are both private functions. That needs to be changed to protected. See comment #174
  2. +++ b/core/includes/bootstrap.inc
    @@ -106,7 +106,9 @@
    -define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
    +if (!defined('REQUEST_TIME')) {
    +  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
    +}
    
    @@ -134,7 +136,9 @@
    -define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
    +if (!defined('DRUPAL_ROOT')) {
    +  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
    +}
    

    These two changes need to be removed from the patch. See comment #181.

  3. In the file core/phpunit.xml.dist are in the testsuite part named "unit" the following two lines added:
    +++ b/core/phpunit.xml.dist
    @@ -10,10 +10,10 @@
    +      <directory>../modules/*/tests/src/Unit</directory>
    +      <directory>../sites/*/modules/*/tests/src/Unit</directory>
    

    These two lines are also needed in the new testsuite named "functional". With one minor change. See comment #177.

    +      <directory>../modules/*/tests/src/Functional</directory>
    +      <directory>../sites/*/modules/*/tests/src/Functional</directory>
    

Who will help?
After these changes will I do a good review.

alexpott’s picture

@daffie fixing #182.2 will break running tests in you just run all the phpunit tests. Splitting them into testsuites does not fix that.

The biggest problem we have is that the test running the new functional test through the UI fails. In fact we've managed to reproduce @benjy's problems on testbot. It is working through run-tests.sh but not the UI.

alexpott’s picture

Re ##182.1 BrowserTestBase is just following TestBase - I think this is fine and the methods should remain private.

alexpott’s picture

Actually wrt. to #183 and #182.2 this problem is pretty insurmountable as long as we allow side effects in unit tests. if I run all the tests together then I get...

PHP Fatal error:  Cannot redeclare t() (previously declared in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/views/tests/src/Unit/EntityViewsDataTest.php:787) in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/bootstrap.inc on line 449

So the long and the short of it is that we need to run the functional and unit test suites completely separately right now.

alexpott’s picture

I've done some testing in #2095771: alexpott's test issue - something is failing in BrowserTestBase::setUp pretty hard. Basically we're getting no test results when we try to run the test through the simpletest ui. Using exceptions I've proved that SIMPLETEST_BASE_URL is fine both when run from run-tests.sh and from the UI. And the error occurs somewhere between the check that SIMPLETEST_BASE_URL is set and the first drupalGet() in BrowserTestBaseTest. So it must be set up - but what? atm no idea.

The quickest way to find out would be to work with @`basic or @isntall to get access to the testbot logs whilst running the patch in https://www.drupal.org/node/2095771#comment-9640255 - but they are busy people.

daffie’s picture

Did some work with the testbot. The error that I got from the testbot is:

Catchable fatal error: Argument 3 passed to Drupal\Core\Database\Database::addConnectionInfo() must be of the type array, null given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/src/BrowserTestBase.php on line 1071 and defined in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Database.php on line 256

Hopefully this is helpful to some testbot guru. :)

Dries’s picture

I think this is a good idea but I'm worried that it introduces a lot of distractions (people will have to learn about this tool, people will be inclined to rewrite existing tests, etc). In the end, I think the impact is bigger than the disruption so let's go ahead and commit it. But please, let's not embark on a whole-scale conversion of existing tests until after 8.0. Let's just use it for things we couldn't test before. Can we agree on that?

daffie’s picture

But please, let's not embark on a whole-scale conversion of existing tests until after 8.0.

@Dries: Can we start whole-scale conversing in 8.1 or do you want to wait for 9.0?

webchick’s picture

8.1.x would be fine. Tests aren't part of the public API.

daffie’s picture

+1. Then I can completely agree with Dries' proposal.

daffie’s picture

Some observations after testing:

  1. After some more testing I found out that trying to run the Functional test with an empty database and no settings.php file will result in errors.
  2. When you start a phpunit test the parameters from run-tests.sh are lost.
  3. Somehow there are problems Drupal service container being empty.
alexpott’s picture

Just learnt about @runTestsInSeparateProcesses all functional tests are going to need that otherwise we'll have problems with running multiple functional tests using the phpunit command. Also we should open an issue to add that tag to all phpunit tests which add functions to the global namespace. See https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi... for more.

larowlan’s picture

alexpott++ that is the secret sauce

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.4 KB
86.6 KB
1.9 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,229 pass(es), 3 fail(s), and 0 exception(s). View

@daffie pointed out that when the parent site was not installed the functional test was failing! So we need to somehow get the db connection in the phpunit environment.

daffie’s picture

Status: Needs work » Needs review
FileSize
87.49 KB
1.9 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 104 pass(es). View

The tests with no parent site are failing for me.
I made two changes:

  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1379 @@
    +    \Drupal::setContainer(NULL);
    

    Changed this line to \Drupal::unsetContainer();.
    Part of #2363341: Throw exception in Drupal::service() and friends, if container not initialized yet..

  2. In the file core/tests/bootstrap.inc I made the folowing change:
    +++ b/core/tests/bootstrap.inc
    @@ -82,7 +82,9 @@ function drupal_phpunit_register_extension_dirs(Composer\Autoload\ClassLoader $l
     drupal_phpunit_register_extension_dirs($loader, $dirs);
    
     // Look into removing these later.
    -define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
    +if (!defined('REQUEST_TIME')) {
    +  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
    +}
    
     // Set sane locale settings, to ensure consistent string, dates, times and
     // numbers handling.
    
    

Lets try again!

daffie’s picture

FileSize
1.9 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 104 pass(es). View
87.57 KB
+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -1060,15 +1063,49 @@ private function changeDatabasePrefix() {
-        throw new Wobbly();
+        throw new \InvalidArgumentException('Invalid --dburl. Minimum requirement: driver://host/database.');

I am not sure what the exact namespace is for a Wobbly. So I am changing it to throw an InvalidArgumentException. :)

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The last two patches are mine. But the changes are very small.
It all looks good to me.
All documentation is in order.
There are two new libraries in this patch. We have a Dries okay for that.
The testbot gives it all right.
So it gets a RTBC from me.

daffie’s picture

Issue summary: View changes

Added alexpott to the suggested commit message.

daffie’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.9 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,226 pass(es). View
87.25 KB

Opps wrong patch. Should have removed this part earlier:

diff --git a/core/scripts/run-tests.sh b/core/scripts/run-tests.sh
index 7bf821c..0f8c330 100644
--- a/core/scripts/run-tests.sh
+++ b/core/scripts/run-tests.sh
@@ -80,6 +80,10 @@
 for ($i = 0; $i < $args['repeat']; $i++) {
   $tests_to_run = array_merge($tests_to_run, $test_list);
 }
+$tests_to_run = [
+  'Drupal\simpletest\Tests\SimpleTestBrowserTest',
+  'Drupal\Tests\simpletest\Functional\BrowserTestBaseTest',
+];

 // Execute tests.
 simpletest_script_execute_batch($tests_to_run);
daffie’s picture

Status: Needs review » Reviewed & tested by the community

The last two patches are mine. But the changes are very small.
It all looks good to me.
All documentation is in order.
There are two new libraries in this patch. We have a Dries okay for that.
The testbot gives it all right.
So it gets a RTBC from me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think we should create a separate issue to add the vendor components. We can get that committed now that we have approval from Dries and then the patch on this issue becomes more reviewable.

I think we need at least some discussion of the solution to the problem. Adding database parameters to an environmental variable is kinda icky.

[Edit: for some unknown reason I asked @daffie to create the new issue - of course anyone can do it :)]

phenaproxima’s picture

Status: Needs work » Needs review
Related issues: +#2433009: Add Mink, with Goutte driver, to core

Created a sub-issue. (This is the first issue I've filed in core, so I apologize if anything is missing from it.)

hussainweb’s picture

@phenaproxima: Thanks for creating the issue. I just tweaked the IS a little bit. It was all good. :)

The patch is there in #2433009: Add Mink, with Goutte driver, to core and under tests. I don't expect it to fail. We can come back to this as soon as that goes in?

daffie’s picture

Status: Needs review » Postponed

On getting the libraries behat/mink and behat/mink-goutte-driver committed in #2433009: Add Mink, with Goutte driver, to core.

daffie’s picture

FileSize
65.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,300 pass(es), 4 fail(s), and 0 exception(s). View
1.38 KB

In the current patch is the settings.php file first checked to see if there are database connections. If there are none then the script will look if there is a script argument dburl. This is of course the wrong sequence.

This patch should test green after #2433009: Add Mink, with Goutte driver, to core lands.

alexpott’s picture

@daffie I debated #208 when I wrote the code. Reading run-tests.sh I now think you've got it right...

  // However, in case no Drupal installation exists, this default database
  // connection can be set and/or overridden with the --dburl parameter.
  if (!empty($args['dburl'])) {
    // Remove a possibly existing default connection (from settings.php).
    Database::removeConnection('default');
alexpott’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -0,0 +1,1384 @@
+        'prefix' => array(
+          'default' => $this->databasePrefix,
+        ),

This should not be added here then since this is done later. This means that "simpletest_original_default" connection is then correct.

daffie’s picture

Status: Postponed » Needs review
daffie’s picture

Status: Needs work » Needs review
FileSize
65.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,302 pass(es). View
988 bytes

Changes:

  1. Removed the part that alexpott requested.
  2. Removed the defined check on SIMPLETEST_DB.
  3. Removed an extra line from core/phpunit.xml.dist.
alexpott’s picture

+++ b/core/modules/simpletest/simpletest.module
@@ -238,6 +238,32 @@ function simpletest_phpunit_configuration_filepath() {
+  if ($db_info['default']['driver'] == 'sqlite') {
+    $db_url = $db_info['default']['driver'] . '://localhost/' . $db_info['default']['database'];
+  }
+  else {
+    $user = '';
+    if ($db_info['default']['username']) {
+      $user = $db_info['default']['username'];
+      if ($db_info['default']['password']) {
+        $user .= ':' . $db_info['default']['password'];
+      }
+      $user .= '@';
+    }
+
+    $db_url = $db_info['default']['driver'] . '://' . $user . $db_info['default']['host'];
+    if (isset($db_info['default']['port'])) {
+      $db_url .= ':' . $db_info['default']['port'];
+    }
+    $db_url .= '/' . $db_info['default']['database'];
+  }
+  if ($db_info['default']['prefix']['default']) {
+    $db_url .= '#' . $db_info['default']['prefix']['default'];
+  }

I think we should encapsulate this on the database class or some other and also the code in run-tests.sh that does the of this opposite code .

alexpott’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -0,0 +1,1381 @@
+      $info = parse_url($db_url);
+      if (!isset($info['scheme'], $info['host'], $info['path'])) {
+        throw new \InvalidArgumentException('Invalid --dburl. Minimum requirement: driver://host/database.');
+      }
+      $info += array(
+        'user' => '',
+        'pass' => '',
+        'fragment' => '',
+      );
+      if ($info['path'][0] === '/') {
+        $info['path'] = substr($info['path'], 1);
+      }
+      if ($info['scheme'] === 'sqlite' && $info['path'][0] !== '/') {
+        $info['path'] = DRUPAL_ROOT . '/' . $info['path'];
+      }
+      $database = array(
+        'driver' => $info['scheme'],
+        'username' => $info['user'],
+        'password' => $info['pass'],
+        'host' => $info['host'],
+        'database' => $info['path'],
+      );
+      if (isset($info['port'])) {
+        $database['port'] = $info['port'];
+      }

This is the same code as we have in run-tests.sh too :)

daffie’s picture

@alexpott: I understand your willingness to combine similar code into a single implementation. The problem as far as I can see is as follows:

  1. The code from the function simpletest_phpunit_configuration_filepath() file the simpletest.module differs from the other two implementations, because this one starts with the Database::getConnectionInfo() and returns a dburl string. The other two do the opposite.
  2. The remaining two implementations are the one in the script run-tests.sh and the one in BrowserTestBase::changeDatabasePrefix(). The problem here is that from the script you cannot access the BrowserTestBase class and vice versa.
alexpott’s picture

FileSize
7.24 KB
68.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,287 pass(es). View

@daffie the patch attached is what I was getting at. Manipulating database connection info is the responsibility of the Database class not run-tests or browsertestbase or simpletest.module.

alexpott’s picture

FileSize
2.55 KB
68.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,303 pass(es). View

Let's keep DRUPAL_ROOT out of the Database class.

neclimdul’s picture

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -443,4 +443,79 @@ public static function closeConnection($target = NULL, $key = NULL) {
+      throw new \InvalidArgumentException('Invalid --dburl. Minimum requirement: driver://host/database.');

+++ b/core/scripts/run-tests.sh
@@ -413,35 +413,12 @@ function simpletest_script_setup_database($new = FALSE) {
+      simpletest_script_print_error('Invalid --dburl. Reason: ' . $e->getMessage());

The --dburl part of this exception should only live in run-test. This is a problem in both versions of the patch.

alexpott’s picture

FileSize
700 bytes
68.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,294 pass(es). View

@neclimdul nice catch. Now who wants to write some nice unit tests for these methods... actually that could be a nice followup.

daffie’s picture

alexpott’s picture

FileSize
68.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,299 pass(es). View
1.77 KB

Couple of minor things I noticed whilst reviewing the patch.

webchick’s picture

pfrenssen’s picture

Status: Needs review » Needs work

Finally found the time to review this in earnest. I reviewed everything in detail except WebAssert, BrowserTestBaseTest, and a part of BrowserTestBase (got up to GetRandomGenerator()). I have no time left unfortunately. Hopefully I can continue the review tomorrow.

  1. +++ b/core/includes/bootstrap.inc
    @@ -841,7 +845,9 @@ function drupal_valid_test_ua($new_prefix = NULL) {
       // Perform a basic check on the User-Agent HTTP request header first. Any
       // inbound request that uses the simpletest UA header needs to be validated.
    

    This documentation could use an update. It only mentions detecting a test run by checking the user agent, but we can now also indicate a test run by setting a cookie.

    It's interesting to document this because this change now allows third party test frameworks to make use of test diagnostics such as reporting of PHP errors.

  2. +++ b/core/includes/bootstrap.inc
    @@ -841,7 +845,9 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    -  if (isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^(simpletest\d+);(.+);(.+);(.+)$/", $_SERVER['HTTP_USER_AGENT'], $matches)) {
    +  $http_user_agent = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : NULL;
    +  $user_agent = isset($_COOKIE['SIMPLETEST_USER_AGENT']) ? $_COOKIE['SIMPLETEST_USER_AGENT'] : $http_user_agent;
    +  if (isset($user_agent) && preg_match("/^(simpletest\d+);(.+);(.+);(.+)$/", $user_agent, $matches)) {
    

    Maybe 'SIMPLETEST_USER_AGENT' is not the best name for this cookie, especially since it will not even be used by Simpletest, but only by Mink, and in the future by Behat.

    I guess this is chosen to be consistent with the regex check on the user agent. We cannot change the contents of the user agent anyway since this would be an API change.

    It would be good to rename this cookie and user agent string for D9.

  3. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -443,4 +443,85 @@ public static function closeConnection($target = NULL, $key = NULL) {
    +    if ($info['path'][0] === '/') {
    +      $info['path'] = substr($info['path'], 1);
    +    }
    +    if ($info['scheme'] === 'sqlite' && $info['path'][0] !== '/') {
    +      $info['path'] = $root . '/' . $info['path'];
    +    }
    

    Can we get a bit of documentation to explain this weird "strip the leading slash and then check if we have another leading slash and are using SQLite" logic?

    This code has been factored out from the run-tests.sh script. In that script this is explained in the documentation for the --db-url option: this allows to make a distinction between relative and absolute paths for SQLite databases. However now that this code is divorced from run-tests.sh it is no longer clear.

  4. +++ b/core/modules/simpletest/simpletest.module
    @@ -238,6 +238,9 @@ function simpletest_phpunit_configuration_filepath() {
    +  // Set the up an environment variable containing the database connection so
    +  // that functional tests can connect to the database.
    +  putenv('SIMPLETEST_DB=' . Database::getConnectionInfoAsUrl());
    

    Set (-the +)up an environment

  5. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +/**
    + * Test case for typical Drupal tests.
    + *
    

    "typical Drupal tests" lol?

  6. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Time limit in seconds for the test.
    +   */
    +  protected $timeLimit = 500;
    

    @var int

  7. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * The root user.
    +   *
    +   * @var UserSession
    +   */
    +  protected $rootUser;
    

    @var \Drupal\Core\Session\UserSession

  8. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * The config directories used in this test.
    +   */
    +  protected $configDirectories = array();
    

    @var array

  9. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Mink session manager.
    +   *
    +   * @var Mink
    +   */
    +  protected $mink;
    

    @var \Behat\Mink\Mink

  10. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +    $port = (isset($parsed_url['port']) ? $parsed_url['port'] : 80);
    

    These outer parentheses are useless.

  11. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +    if ($path == '/') {
    +      $path = '';
    +    }
    

    This doesn't seem to be needed. When $path is set the trailing slashes are trimmed with rtrim(), so it will never contain '/'.

  12. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +        if (trim($key, ' ') == 'idekey') {
    +          $session->setCookie('XDEBUG_SESSION', trim($value, ' '));
    +        }
    

    Both of these calls to trim() do not need the ' ' argument, by default trim() will strip whitespace.

  13. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +   * @param string|null $name
    +   *   (optional) Name of the session OR active session will be used.
    

    This reads a bit strange. Better is "(optional) Name of the session. Defaults to the active session."

  14. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Returns Mink assert session.
    +   *
    

    This is not a session object. Maybe just say "Returns Mink assert object".

  15. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +   * @param string $path
    +   *   Drupal path or URL to load into internal browser
    

    Not strictly the internal browser as was the case with Simpletest. This may well be an external browser controlled via Selenium web driver.

    Also missing period.

  16. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +    if ($result === SAVED_NEW) {
    +      // Grant the specified permissions to the role, if any.
    +      if (!empty($permissions)) {
    +        user_role_grant_permissions($role->id(), $permissions);
    +        $assigned_permissions = entity_load('user_role', $role->id())->getPermissions();
    +        $missing_permissions = array_diff($permissions, $assigned_permissions);
    +        if ($missing_permissions) {
    +          $this->fail(String::format('Failed to create permissions: @perms', array('@perms' => implode(', ', $missing_permissions))));
    +        }
    +      }
    +      return $role->id();
    +    }
    +    else {
    +      return FALSE;
    +    }
    

    The else is not needed, just return FALSE.

  17. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +    // To prevent the introduction of random test failures, ensure that the
    +    // returned string contains a character that needs to be escaped in HTML by
    +    // injecting an ampersand into it.
    +    $replacement_pos = floor($length / 2);
    

    YES!!!!

  18. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Logs a user out of the internal browser and confirms.
    +   *
    +   * Confirms logout by checking the login page.
    

    Not the internal browser, the Mink browser.

  19. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +      $form = $assert_session->elementExists('xpath', "//form[@id='" . $form_html_id . "']");
    

    If we are using double quotes you can put the $form_html_id variable inline, no need to concatenate the different parts of the string.

  20. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +   * @param NodeElement|string $select
    +   *   Name, ID, or Label of select field to assert.
    

    @param \Behat\Mink\Element\NodeElement

  21. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +   * @param Element $container
    +   *   (optional) Container element to check against. Defaults to current page.
    

    @param \Behat\Mink\Element\Element

  22. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  public function run(\PHPUnit_Framework_TestResult $result = NULL) {
    +    if ($result === NULL) {
    +      $result = $this->createResult();
    +    }
    +
    +    parent::run($result);
    +    return $result;
    +  }
    

    Is this method needed at all? The parent method also will call $this->createResult() if $result is NULL.

  23. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Generates a unique random string containing letters and numbers.
    +   *
    +   * Do not use this method when testing unvalidated user input. Instead, use
    +   * \Drupal\simpletest\TestBase::randomString().
    +   *
    +   * @param int $length
    +   *   (optional) Length of random string to generate.
    +   *
    +   * @return string
    +   *   Randomly generated unique string.
    +   *
    +   * @see \Drupal\Component\Utility\Random::name()
    +   */
    +  public function randomName($length = 8) {
    +    return $this->getRandomGenerator()->name($length, TRUE);
    +  }
    

    This is a duplicate of randomMachineName(). I would suggest to remove this and keep randomMachineName().

  24. +++ b/core/modules/simpletest/src/TestDiscovery.php
    @@ -408,6 +410,31 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
    +      else if ($namespace[3] == 'Unit') {
    

    Coding standards: use 'elseif' instead of 'else if'.

  25. +++ b/core/modules/simpletest/src/TestDiscovery.php
    @@ -408,6 +410,31 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
    +        // A module unit test
    

    Missing period.

  26. +++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
    @@ -138,11 +138,19 @@ public function testTestingThroughUI() {
    +
    +    $this->drupalGet('admin/config/development/testing');
    +    $edit = array(
    +      // A PHPUnit functional test.
    +      'tests[Drupal\Tests\simpletest\Functional\BrowserTestBaseTest]' => TRUE,
    +    );
    +    $this->drupalPostForm(NULL, $edit, t('Run tests'));
    +    $this->assertText('0 fails, 0 exceptions');
    

    This is duplicating code, we can put the different tests in an array and loop over it.

    The documentation of this method also needs to be updated.

pfrenssen’s picture

Continued review:

  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +    $filename = $this->siteDirectory . '/settings.php';
    +
    +    error_log($filename);
    

    This call to error_log() seems to be debugging cruft.

  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Resets all data structures after having enabled new modules.
    +   *
    +   * This method is called by \Drupal\simpletest\WebTestBase::setUp() after
    +   * enabling the requested modules. It must be called again when additional
    +   * modules are enabled later.
    +   */
    

    This method is now called by BrowserTestBase::setUp().

  3. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Refreshes in-memory configuration and state information.
    +   *
    +   * Useful after a page request is made that changes configuration or state in
    +   * a different thread.
    +   *
    +   * In other words calling a settings page with $this->drupalPostForm() with a
    +   * changed value would update configuration to reflect that change, but in the
    +   * thread that made the call (thread running the test) the changed values
    +   * would not be picked up.
    +   *
    +   * This method clears the cache and loads a fresh copy.
    +   */
    

    This gives $this->drupalPostForm() as an example but we won't be using this any more. The new method is called $this->submitForm().

  4. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -0,0 +1,1355 @@
    +  /**
    +   * Returns whether a given user account is logged in.
    +   *
    +   * @param \Drupal\user\UserInterface $account
    +   *   The user account object to check.
    +   *
    +   * @return bool
    +   *   Return TRUE if the user is logged in, FALSE otherwise.
    +   */
    +  protected function drupalUserIsLoggedIn($account) {
    

    Let's add a type hint to $account.

  5. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -0,0 +1,71 @@
    +   * @param \Behat\Mink\Element\TraversableElement $container
    +   *   The document to check against.
    

    This is optional and defaults to the current page. Wouldn't be bad to mention this since this will be the example that all upcoming test methods will be based on.

  6. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -0,0 +1,71 @@
    +    if (NULL === $node) {
    +      throw new ElementNotFoundException($this->session, 'button', 'id|name|label|value', $button);
    +    }
    

    Very minor but Yoda conditions are nowhere else used in D8 core.

  7. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
    @@ -0,0 +1,64 @@
    +/**
    + * @file
    + * Definition of \Drupal\Tests\simpletest\Functional\BrowserTestBaseTest.
    + */
    

    We usually say "Contains..." and not "Definition of...".

  8. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
    @@ -0,0 +1,64 @@
    +/**
    + * Tests BrowserTestBase functionality.
    + *
    + * @group simpletest
    + * @group modern
    + *
    + * @runTestsInSeparateProcesses
    + */
    

    Is this "@modern" group intentional, or is this a leftover from debugging?

  9. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
    @@ -0,0 +1,64 @@
    +    // Ensure the form and text field exist.
    +    $this->assertSession()->elementExists('css', 'form#form-test-form-test-object');
    +    $this->assertSession()->fieldExists('bananas');
    

    SWEET!!!

daffie’s picture

FileSize
68.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,636 pass(es). View

Thank you for the extensive review pfrenssen!
I have tried to make an interdiff.txt file, but I did not succeed. :(

  1. 224.1 I have changed the text to: "Perform a basic check on the Simpletest-user-agent cookie first. If it does not exists then perform a basic check on the User-Agent HTTP request header. Any inbound request that uses the simpletest UA header needs to be validated." Hope you approve.
  2. 224.2 Lets wait to drupal 9.x where we can rename the module and the user-agent cookie name.
  3. 224.3 Changed it to:
        // A SQLite database is special because it datebase connection is directly
        // to the database file. The variable $info['path'] holds the location of
        // the database file.
        if ($info['scheme'] === 'sqlite') {
          if ($info['path'][0] !== '/') {
            $info['path'] = '/' . $info['path'];
          }
          $info['path'] = $root . $info['path'];
        }
        elseif ($info['path'][0] === '/') {
          $info['path'] = substr($info['path'], 1);
        }
    
  4. 224.18 Renamed "internal browser" to "mink controled browser"
  5. 224.26 Changed it to:
        $tests = array(
          // A KernalTestBase test.
          'Drupal\field\Tests\String\StringFormatterTest',
          // A PHPUnit unit test.
          'Drupal\Tests\action\Unit\Menu\ActionLocalTasksTest',
          // A PHPUnit functional test.
          'Drupal\Tests\simpletest\Functional\BrowserTestBaseTest',
        );
    
        foreach($tests as $test) {
          $this->drupalGet('admin/config/development/testing');
          $edit = array(
            "tests[$test]" => TRUE,
          );
          $this->drupalPostForm(NULL, $edit, t('Run tests'));
          $this->assertText('0 fails, 0 exceptions');
        }
    
  6. All others from 224 and the ones of 225. Good finds and I have fixed them.
daffie’s picture

Status: Needs work » Needs review

For the testbot.

pfrenssen’s picture

FileSize
18.82 KB

Here's the interdiff between #222 and #226.

Off-topic: you can generate these interdiffs easily if you create a branch before you start working:

# Start off by updating Drupal to the latest version.
$ git checkout 8.0.x
$ git pull

# Create a new branch to work in. I use the issue number.
$ git checkout -b 2232861

# Download, apply and commit the latest patch. I use the comment number as commit message.
$ wget https://www.drupal.org/files/issues/2232861-db.222.patch
$ patch -p1 < 2232861-db.222.patch
$ git add .
$ git commit -m "222"

# Now make your changes.
$ work work work

# Save the interdiff and the new patch, done!
$ git diff > interdiff.txt
$ git diff 8.0.x > 2232861-226.patch

In case you want to generate an interdiff between two patches, just make a branch for each patch and generate the diff between them:

# Download both patches.
$ wget https://www.drupal.org/files/issues/2232861-db.222.patch https://www.drupal.org/files/issues/2232861-226.patch

# Create a new branch based on 8.0.x and commit the first patch.
$ git checkout -b 2232861-222 8.0.x
$ patch -p1 < 2232861-db.222.patch
$ git commit -am "222"

# Create a second branch and commit the second patch.
$ git checkout -b 2232861-226 8.0.x
$ patch -p1 < 2232861-226.patch
$ git commit -am "226"

# Save the diff between the two branches in a file.
$ git diff 2232861-222..2232861-226 > interdiff.txt
daffie’s picture

@pfrenssen: Thank you for explaining. Going to use git to make interdiffs in the future. I was only using the interdiff command.

pfrenssen’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/bootstrap.inc
    @@ -843,8 +843,10 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    -  // Perform a basic check on the User-Agent HTTP request header first. Any
    -  // inbound request that uses the simpletest UA header needs to be validated.
    +  // Perform a basic check on the Simpletest-user-agent cookie first. If it
    +  // does not exists then perform a basic check on the User-Agent HTTP request
    +  // header. Any inbound request that uses the simpletest UA header needs to be
    +  // validated.
    

    Let's explain the 'why' rather than the 'how'. Maybe something like this:

    "A valid Simpletest request will contain a hashed and salted authentication code. Check if this code is present in a cookie or custom user agent string."

  2. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -469,12 +469,20 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
    +    // A SQLite database is special because it datebase connection is directly
    +    // to the database file. The variable $info['path'] holds the location of
    +    // the database file.
    

    It's special in that it supports both relative and absolute paths, these are indicated by a leading slash. See the examples in the documentation for the '--dburl' parameter in run-tests.sh.

    Maybe this would be better:
    "A SQLite database path with two leading slashes indicates a system path. Otherwise the path is relative to the Drupal root."

  3. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -469,12 +469,20 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
    +    if ($info['scheme'] === 'sqlite') {
    +      if ($info['path'][0] !== '/') {
    +        $info['path'] = '/' . $info['path'];
    +      }
    +      $info['path'] = $root . $info['path'];
         }
    

    By changing this logic it is now no longer possible to use double slashes to provide a system path.

    For example if I run the tests with the option '--dburl sqlite://localhost//home/pieter/db.sql' then the database path is now invalid since it will be prefixed with the Drupal root path.

    The original logic was correct but just needed to have some documentation to explain the strange business with the double slashes.

  4. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -150,7 +153,7 @@
    -   * The current user logged in using the internal browser.
    +   * The current user logged in using the Mink controled browser.
    

    Typo: "controlled" instead of "controled".

  5. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -384,7 +387,7 @@ protected function prepareRequest() {
    -   *   Drupal path or URL to load into internal browser
    +   *   Drupal path or URL to load into Mink controled browser.
    

    Typo "controlled".

  6. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -418,7 +421,7 @@ protected function drupalGet($path, array $options = array()) {
    -   *   A path from the internal browser content.
    +   *   A path from the Mink controled browser content.
    

    Typo "controlled".

  7. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -1033,9 +1005,9 @@ protected function installParameters() {
    +   * SIMPLETEST_USER_AGENT by the mink controled browser. During early Drupal
    

    Typo "controlled".

pfrenssen’s picture

If people have trouble running the tests through PHPUnit, you need to set the environment variable and run the tests as your webserver user. For example if your base URL is 'http://drupal.local' and your webserver runs as the user 'www-data':

$ cd core
$ sudo -u www-data SIMPLETEST_BASE_URL=http://drupal.local ./vendor/bin/phpunit --testsuite functional
daffie’s picture

Status: Needs work » Needs review
FileSize
68.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,681 pass(es). View
4.93 KB

Thank you again for the reviewing pfrenssen.

  1. 230.1 & 230.2 Changed the text to your suggestion.
  2. 230.3 Thought I was smart. Changed it back to what it was.
  3. 230.4 - 230.7 Typo fixed. English is not my native language.
pfrenssen’s picture

Thanks! I reviewed the interdiff and my remarks have all been addressed. This looks to be near completion.

I want to do a few final tests before signing off on this: seeing that the patch in #226 was green despite having broken SQLite support I want to test it with a SQLite database passed as relative and absolute path in run-tests.sh, and I would like to test it without having a preinstalled Drupal installation (using the --sqlite option). Adding SQLite test coverage for run-tests.sh is out of scope for this issue, but I want to at least try it out.

I also intend to do a final review of the entire patch.

pfrenssen’s picture

Manual SQLite tests pass without problems:

[pieter@thinkarch drupal (2232861 $%)]$ sudo -u http php ./core/scripts/run-tests.sh --sqlite /tmp/db.sql --dburl sqlite://localhost//tmp/absolutedb.sql --url http://drupal.dev/drupal --class "Drupal\Tests\simpletest\Functional\BrowserTestBaseTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\simpletest\Functional\BrowserTestBaseTest

Test run started:
  Thursday, March 5, 2015 - 17:06

Test summary
------------

Drupal\Tests\simpletest\Functional\BrowserTestBaseTest         2 passes                                      

Test run duration: 33 sec

[pieter@thinkarch drupal (2232861 $%)]$ sudo -u http php ./core/scripts/run-tests.sh --sqlite /tmp/db.sql --dburl sqlite://localhost/relativedb.sql --url http://drupal.dev/drupal --class "Drupal\Tests\simpletest\Functional\BrowserTestBaseTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\simpletest\Functional\BrowserTestBaseTest

Test run started:
  Thursday, March 5, 2015 - 17:08

Test summary
------------

Drupal\Tests\simpletest\Functional\BrowserTestBaseTest         2 passes                                      

Test run duration: 33 sec
daffie’s picture

There is no SQLite support before #2318191: [meta] Database tests fail on SQLite lands.

pfrenssen’s picture

FileSize
68.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,985 pass(es). View
8.54 KB

I ran this through its paces extensively. I encountered two problems but both are out of scope: test failures messages are not displayed when running the tests through the Simpletest UI, and for some bizarre reason when I enable xdebug the tests finish in a split second and always return green. Without debugger they run fine, and I can debug normal Simpletests without problems. But not BrowserTestBaseTest :-/

I also reviewed the code again and checked coding standards with PHPCodeSniffer. Made some updates to documentation and applied coding standards fixes. Did not touch the code, so I can still RTBC this.

The patch looks ready to me.

Leaving on "Needs review" since we still need to update the issue summary and write a change record.

pfrenssen’s picture

larowlan’s picture

Issue summary: View changes
Issue tags: +DrupalSouth

Tagging for DrupalSouth and added tasks

pfrenssen’s picture

Issue summary: View changes

Added beta phase evaluation.

pfrenssen’s picture

Title: Step 1: Create BrowserTestBase for web-testing on top of Mink » Create BrowserTestBase for web-testing on top of Mink
Assigned: Unassigned » pfrenssen
Issue summary: View changes

Brought issue summary up to the current state. Will start on the change record now.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record
+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -0,0 +1,1326 @@
+    // In order to debug web tests you need to either set a cookie, have the
+    // Xdebug session in the URL or set an environment variable in case of CLI
+    // requests. If the developer listens to connection when running tests, by
+    // default the cookie is not forwarded to the client side, so you cannot
+    // debug the code running on the test site. In order to make debuggers work
+    // this bit of information is forwarded. Make sure that the debugger listens
+    // to at least three external connections.
+    $request = \Drupal::request();
+    $cookie_params = $request->cookies;
+    if ($cookie_params->has('XDEBUG_SESSION')) {
+      $session->setCookie('XDEBUG_SESSION', $cookie_params->get('XDEBUG_SESSION'));
+    }
+    // For CLI requests, the information is stored in $_SERVER.
+    $server = $request->server;
+    if ($server->has('XDEBUG_CONFIG')) {
+      // $_SERVER['XDEBUG_CONFIG'] has the form "key1=value1 key2=value2 ...".
+      $pairs = explode(' ', $server->get('XDEBUG_CONFIG'));
+      foreach ($pairs as $pair) {
+        list($key, $value) = explode('=', $pair);
+        // Account for key-value pairs being separated by multiple spaces.
+        if (trim($key) == 'idekey') {
+          $session->setCookie('XDEBUG_SESSION', trim($value));
+        }
+      }
+    }

Given this code I think @pfrenssen's xdebug problems need further investigation.

Also we are not going to be telling anyone to use BrowserTestBase tests atm so please no change record yet. Nothing has changed - if you want to do functional testing you should still be writing simpletest tests. This is step 1.

pfrenssen’s picture

@alexpott, this part of the code is not the cause of on my inability to debug these tests. Once I enable my debugger all PHPUnit tests are skipped, the test's setup() methods are not even running. I did not look into it since I was testing on my arch linux system which is running the very latest versions of all software packages. It's not unusual for me to encounter weird incompatibilities that are then solved a few days later. I will dig a bit deeper tomorrow and try to find the real cause.

I had already started on the change record, will post my work in progress here so it can perhaps be used as inspiration later:

Since the development of Drupal 7 started a lot of effort was put in providing automated test coverage. Originally the framework used for functional testing in Drupal was based on SimpleTest but over the years many changes were made to it and it has morphed into a mutant beast of difficult to maintain custom code. SimpleTest simulates a browser by doing HTTP requests using cURL and parsing the resulting HTML and HTTP headers. While this works well for testing HTML pages this also means there was no support for testing Javascript code. None of our front-end code was testable. What we need is browser based testing.

Whilst we were off on our island, the PHP community has been working together on solving the problem of browser based testing, and the most prominent result of this is the Mink browser controller. Mink allows you to interface with a range of different browsers or browser emulators, from a simple HTTP based implementation such as Goutte, over a simulated javascript environment such as zombie.js to a full webdriver such as Selenium that allows full control over real browsers.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Issue summary: View changes

Removed contributor task to write a change record.

daffie’s picture

Status: Needs work » Needs review

Putting it back to "need review".

pfrenssen’s picture

Am back from holiday and have been looking into the Xdebug problem.

With the latest Xdebug 2.3.2 on PHP 5.6.6 the problem still occurs: when I enable the Xdebug listener in PHPStorm and start the PHPUnit tests then the tests finish instantly and are always green. What is interesting is that if I am not listening on the Xdebug port the tests work fine. I am using the option xdebug.remote_autostart so an Xdebug session should always be present with every request.

I updated PHPUnit to 4.4.4 but this did not help.

I started downgrading to older versions of PHP and Xdebug and apparently it only happens on PHP 5.6, once I get down to PHP 5.5 I can debug again:

PHP version Xdebug version Test result
5.5.16 2.2.5 PASS
5.6.0 2.2.5 FAIL
5.6.5 2.2.7 FAIL
5.6.6 2.3.2 FAIL

It seems like this issue is not to blame, but rather the problem seems to be with Xdebug on PHP 5.6. I'll start digging to see if I can pinpoint where exactly it goes wrong.

alexpott’s picture

So I think we have an @pfrenssen environment issue. I can run PHPUnit with xdebug on all of those PHP version. I don't use xdebug.remote_autostart

Added #2459155: Remove REQUEST_TIME from bootstrap.php to deal with REQUEST_TIME

mpdonadio’s picture

Per http://xdebug.org/docs/all_settings, the default value for xdebug.remote_autostart is 0, which may limit the impact of the problem.

pfrenssen’s picture

That setting doesn't change anything for me, if I turn xdebug.remote_autostart off and start the debugging session manually the problem still occurs.

I have been reviewing the build options of PHP and Xdebug on Arch Linux but I do not see anything out of the ordinary. It is built from source with generic options. I tried with and without profiling (option xdebug.coverage_enable) but this does not change anything.

I am using the following extensions:

  • curl
  • gd
  • gettext
  • iconv
  • mysqli
  • mysql
  • openssl
  • pdo_mysql
  • phar
  • posix
  • soap
  • zip

I can replicate the problem using this command, note that my web server runs as the user "http":

sudo -u http SIMPLETEST_BASE_URL=http://drupal.local/drupal ./vendor/bin/phpunit --testsuite functional

Or, with xdebug.remote_autostart disabled:

sudo -u http XDEBUG_CONFIG="idekey=PHPSTORM" SIMPLETEST_BASE_URL=http://drupal.local/drupal ./vendor/bin/phpunit --testsuite functional

When I enable the listener in PHPStorm the tests finish instantly which isn't normal, it should run around a minute or so.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Anyway since this debugging problem is unique to my environment this shouldn't hold up this issue any more. I was already satisfied with the state of the patch in comment #236. The issue summary has been updated and a change record is not needed as per #241.

This means this is ready!!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Let's wait for #2459155: Remove REQUEST_TIME from bootstrap.php so we don't have to do the define wrapping.

pfrenssen’s picture

Status: Needs review » Postponed
Related issues: +#2459155: Remove REQUEST_TIME from bootstrap.php
FileSize
68.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2232861-251.patch. Unable to apply patch. See the log in the details link for more information. View

Postponing on #2459155: Remove REQUEST_TIME from bootstrap.php.

I have tested it in combination with that issue and it works perfect. I had to solve a merge conflict, am uploading the rerolled patch so we do not have to fix this again once #2459155: Remove REQUEST_TIME from bootstrap.php is in.

Note that this will fail now, but once #2459155: Remove REQUEST_TIME from bootstrap.php is in this can be retested and should turn green.

pfrenssen’s picture

Status: Postponed » Needs review
pfrenssen’s picture

Status: Needs work » Needs review
FileSize
68.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,220 pass(es). View

Rebased on latest HEAD.

pfrenssen’s picture

FileSize
67.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,218 pass(es). View
513 bytes

... and removed the if(!defined('REQUEST_TIME')) wrapper.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/bootstrap.inc
@@ -114,7 +114,9 @@
+if (!defined('DRUPAL_ROOT')) {
+  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+}

Is this actually necessary? And if so why?

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
67.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,223 pass(es). View
466 bytes

No that shouldn't be needed.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.71 KB
67.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,344 pass(es). View

We need another annotation to allow all PHPUnit test suites to run at the same time. See https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...

Now if you have SIMPLETEST_BASE_URL and SIMPLETEST_DB this will run all the tests.

Patch also improves error messages and removes usage of Drupal\Component\Utility\String

Before this patch if phpunit and the webserver were being run by the different users you would see the following error:

1) Drupal\Tests\simpletest\Functional\BrowserTestBaseTest::testGoTo
chmod(): Operation not permitted

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/src/BrowserTestBase.php:303
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:925
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:934
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:934
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:934
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/src/BrowserTestBase.php:326
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/src/BrowserTestBase.php:337

With the new patch:

1) Drupal\Tests\simpletest\Functional\BrowserTestBaseTest::testGoTo
Can not make sites/simpletest/903748/files/php/twig writeable whilst cleaning up test directory. The webserver and phpunit are probably not by run by the same user.

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/src/BrowserTestBase.php:305
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:925
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:934
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:934
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/file.inc:934
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/src/BrowserTestBase.php:329
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/simpletest/src/BrowserTestBase.php:340
pfrenssen’s picture

FileSize
6.6 KB

We need another annotation to allow all PHPUnit test suites to run at the same time. See https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...

Now if you have SIMPLETEST_BASE_URL and SIMPLETEST_DB this will run all the tests.

This was confusing me but it appears this is missing from the interdiff. Here's the complete interdiff between #258 and #260.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community
  1. We need another annotation to allow all PHPUnit test suites to run at the same time. See https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...

    Now if you have SIMPLETEST_BASE_URL and SIMPLETEST_DB this will run all the tests.

    This is indeed needed, we want to run our entire PHPUnit test suite in one go on the test bots, and without disabling @preserveGlobalState we would get notices about constants that are already defined.

    WIth that annotation in place I am able to successfully run the entire suite:

    $ cd core
    $ sudo -u http SIMPLETEST_BASE_URL=http://drupal.local SIMPLETEST_DB=mysql://drupal:drupal@localhost/drupal ./vendor/bin/phpunit
    PHPUnit 4.4.2 by Sebastian Bergmann.
    
    Configuration read from /home/pieter/drupal/core/phpunit.xml.dist
    
    .............................................................   61 / 7650 (  0%)
    (snip)
    ............................................................. 7625 / 7650 ( 99%)
    .........................
    
    Time: 1.44 minutes, Memory: 223.50Mb
    
    OK, but incomplete, skipped, or risky tests!  
    Tests: 7650, Assertions: 14305, Incomplete: 2.
    
  2. If I "forget" to run the tests as the webserver user I get the message that "The webserver and phpunit are probably not by run by the same user". This is very helpful.
  3. If I omit to set SIMPLETEST_DB, and remove my database credentials from settings.php, then I get the helpful error message that I need to set SIMPLETEST_DB. Since it would otherwise not be obvious what the problem is this is a very nice addition.
  4. The other changes also look good. The String class is deprecated because it will become a reserved keyword in PHP7. The environment variables are now suggested rather than having garbage defaults.

Thanks a lot @alexpott! This looks really good now.

grom358’s picture

@alexpott and @pfrenssen I haven't gotten back to this issue in awhile. Good to see people are still keeping this patch going. In regards to comment #257 I think that had something todo with earlier on during test detection it would try to define that constant again when BrowserTestBase bootstrap after having ran a unit test that defined that constant. So probably unnecessary now and if I recall correctly DRUPAL_ROOT has largely been replaced with asking the container for the drupal root instead of global constant.

Wim Leers’s picture

The notes in #262 are extremely helpful and make me go into "can't wait to use this" mode :)

alexpott’s picture

@Wim Leers - a note of caution about the

"can't wait to use this" mode

This is step 1 - we need to lots of work integrating how we report test results into our existing test infra.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think it is acceptable for me to commit this patch because I have written very little of it. My contributions have been tidying up around the edges - for example ensuring that running PHPUnit from the CLI works as expected and reports good errors when it does not. I've also ensured that this patch does not make unnecessary changes to runtime code. Also this is the first step - we have some ways to go before we can deprecate simpletest and use this to replace that - let's get on with that. The dream of javascript testing and decoupling our tests from code using behat is extremely worthwhile and there is no point being held up on step 1.

Committed 300f14e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to issue summary.

  • alexpott committed 300f14e on 8.0.x
    Issue #2232861 by grom358, daffie, alexpott, larowlan, pfrenssen,...
Xano’s picture

I'd like to thank the contributors to this issue for all their time and efforts :) I've only lurked here, but enough to realize it's been a lot of work getting this into core.

phenaproxima’s picture

I second #268. Congratulations to everyone who worked on this patch!

jibran’s picture

This is a start now we need a big big meta issue to convert all the WebTest to BrowserTest.

fago’s picture

Wow, that's really great. However, it's left a bit unclear now what this means for people writing functional tests. Should they use the new test base class, or is it likely you run into problems. When would you use it and when not? Are there any docs to help picking the right test base class?

jibran’s picture

chx’s picture

Not disruptive. It adds a new base class which can be used to write browser based tests.

However, it's left a bit unclear now what this means for people writing functional tests. Should they use the new test base class, or is it likely you run into problems. When would you use it and when not?

Should've been postponed to Drupal 8.1. This is a feature.

Edit: or at least BrowserTestBase and WebTestBase needs documentation to very very clearly tell people which one to implement.

larowlan’s picture

Quoting Dries at #188

I think this is a good idea but I'm worried that it introduces a lot of distractions (people will have to learn about this tool, people will be inclined to rewrite existing tests, etc). In the end, I think the impact is bigger than the disruption so let's go ahead and commit it. But please, let's not embark on a whole-scale conversion of existing tests until after 8.0. Let's just use it for things we couldn't test before. Can we agree on that?

So you can use it in contrib if you like, but not in core until 8.1.

larowlan’s picture

Oh and to @grom358, @daffie, @alexpott, @pfrenssen, @hussainweb, @pcambra, @jibran, @phenaproxima, @moshe weitzman, @nick_schuch and @benjy - thanks for all your work on this - see you over in #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary

chx’s picture

or at least BrowserTestBase and WebTestBase needs documentation to very very clearly tell people which one to implement.

There is 0 documentation to this extent. I am sorry for not reading a 200+ comment issue throughly.

larowlan’s picture

larowlan’s picture

/me starts on change record for this issue

larowlan’s picture

larowlan’s picture

Status: Fixed » Needs review

Draft change record is here https://www.drupal.org/node/2469723 - could someone please review?

chx’s picture

Critical doxygen issue filed at #2469731: Document when to use BrowserTestBase.

nick_schuch’s picture

Draft change record looks good @larowlan!

jibran’s picture

Status: Needs review » Fixed

I published the change notice. Maybe we can add some thing related ajax form post methods as well.

larowlan’s picture

There are no Ajax form methods until step 2. I will add that to the feature parity section of the change notice.

RoySegall’s picture

Well... There is a couple of things that this patch don't prove or missing:
1. There is no option to log events during the tests. For example if i'm using the assertSame method the message argument is missing from the test results.
2. When i use the debug function to debug variables then the tests is failing.

Someone else has also got this issue?

larowlan’s picture

1. Can you open a follow up?
2. debug() calls trigger_error() so without the set_error_handler logic in the Simpletest runner to catch and log these - its phpunit, you can just print the output or use xdebug to set a breakpoint in the runner. If you're calling debug() in the child site (not the runner) and its causing failures, then that is an issue - can you open a follow up?

We don't have feature parity with WebTestBase yet.

RoySegall’s picture

1. Yeah sure, i'll open one.
2. No, i don't have a debug inside the site only through the test methods but it's worth checking.

neclimdul’s picture

Sorry for the post commit review but was there a reason this went into core/modules/simpletest/src instead of /core/tests/Drupal/* ? I don't see any connection to simpletest other then the environment variables.

larowlan’s picture

@neclimdul, happy for a follow up to move, can't recall there being any reason

neclimdul’s picture

That's what I figured but I wanted to check first. A bit bare but follow up created. #2473269: Move Mink BrowserTestBase classes into Drupal\Tests

Fabianx’s picture

So I have a new concern with using phpunit to run this new BrowserTestBase:

So far the distinction was pretty clear:

- phpunit - fast, easy, isolated tests -- run before each commit
- junit - fast simple JS tests, tests behavior of one class - run before committing JS code

- mink - JS tests often operating on simple pre-made pages integrating several complex JS together.
- simpletest - Database driven integration tests, let testbot / test engine / jenkins run those, sometimes run locally, quite slow

- behat - behavior driven very slow web tests with full integration, tests mainly behaviors that are specified by a customer.

--

Now if I run phpunit I suddenly startup simpletest, have to set a SIMPLETEST_BASE_URL and need to deal with users, etc.

Also the runs take suddenly 1.02 minutes and 172 MB instead of several seconds before ...

Unit tests _need_ to be fast.

Can we perhaps make a follow-up that if SIMPLETEST_BASE_URL is not set, we don't do all the slow things and just issue a warning or ignore instead of an error?

That would be great ...

dawehner’s picture

Its a common misconception that phpunit === unit tests, I would see phpunit as a testing framework, given that it also powers behat under the hood.

Phpunit itself already has the abstraction of testgroups, of we have two now:

phpunit --testsuite unit // that one works directly
phpunit --testsuite functional // that one needs the environment variable

and once #2304461: KernelTestBaseTNG™ is in, we have a third one.

Fabianx’s picture

#292: Oh thank you!

That is great and I did not know.

Can we update the change notice with that, please?

Thanks!

znerol’s picture

Mile23’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jibran’s picture

klausi’s picture