Create BrowserTestBase for web-testing on top of Mink

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.

Created #2257519: Move content assertion methods into a trait, so DUTB can consume it, too

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.

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

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.)

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

epic.

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.

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.

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

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.

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

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.

Some cleanup and polish.

Small fix

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).

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.

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.

@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

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.

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

Here is something reviewable without vendor libraries.

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.

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

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

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

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.

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.

Overall a great patch! I like it very much.

+++ 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;

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.

My points from comment #69 are still open.

@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.

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

Core only patch

+++ 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?

@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.

Updated drupalUserIsLoggedIn and fix couple other minor things.

Patch reroll

patch reroll

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

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

@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.

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

Reroll and changed to lowerCamel property names

Patch without vendor for review purpose

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

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.

@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.

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 :)

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.

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.

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

@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.

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

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.

@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

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

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

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

It's hussainweb. Thanks!

@grom538: My github account is daffie1.

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

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

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.

Using getEditable from the config factory as @benjy suggests.

@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.

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

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?

@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 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.

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.

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

@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.

+++ 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?

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.

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.

@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

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.

Patch rerolled with invaluable assistance from @grom358.

Setting status to needs review so bot tests rerolled patch.

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

@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.

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.

New libraries are for Dries to approve.

I'm extremely ++ to this change.

@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.

@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.

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

@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.

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

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

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).

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.

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.

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.

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.

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.

@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.

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?

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.

I created #2427191: Test-runner ignores @group annotations for PHPUnit-based tests in modules. for solving the grouping of PHPUnit tests.

#167 / #2427191: Test-runner ignores @group annotations for PHPUnit-based tests in modules. is a sort of duplicate of #2301481: Mark test groups as belonging to modules in UI

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

Re #169: Right, good point!

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.

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);

+++ 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?

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

• 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]

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.

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.

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?

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

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.

@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.

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

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.