Installer: Convert system functional tests to phpunit

Let's see where I get to

Current progress ...

@vaplas
That sounds like the user is not logged in after the installation.

Keep up the great work!

I tried to compare what is different to this patch from before, they are reallly similar at this point!

+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
@@ -0,0 +1,280 @@
+    // Setup Mink.
+    $this->initMink();

One part of the initMink call is

    $session = $this->getSession();
    $session->visit($this->baseUrl);

, so I'm wondering whether skipping this here makes any difference. I think it is worth giving it a try

@lendude++ I'm glad you actually tried it out :)

+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
@@ -205,37 +205,9 @@
+  protected function initFrontPage() {
+    // We don't want to visit the front page with the installer when
+    // initializing Mink, so we do nothing here.
   }

👍 Nice method name and excellent documentation.

#43: I wanted to check that without this correction tests are still falling.

Did you tried that already out in a test only issue?
@vaplas I would rely on you on

1. +++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationTest.php
   @@ -43,31 +43,28 @@ protected function setUp() {
   -    $path = $this->siteDirectory . '/profiles/mydistro';
   +    $path = $this->root . DIRECTORY_SEPARATOR . $this->siteDirectory . '/profiles/mydistro';
   ...
   +    // Place a custom local translation in the translations directory.
   +    mkdir(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations', 0777, TRUE);
   +    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.de.po', $this->getPo('de'));
   

   Note: One is using \Drupal::root() and one $this->root, but mostly i'm wondering whether why we cannot keep the code in visitInstaller

2. +++ b/core/tests/Drupal/Tests/Core/Test/SkippedDeprecationsInheritanceTest.php
   @@ -0,0 +1,25 @@
   +class SkippedDeprecationsInheritanceTest extends UnitTestCase {
   

   Does this belong in this patch?

1. +++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationQueryTest.php
   @@ -43,22 +43,19 @@ protected function setUp() {
   +    $path = $this->root . DIRECTORY_SEPARATOR . $this->siteDirectory . '/profiles/mydistro';
   ...
   +    mkdir(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations', 0777, TRUE);
   +    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.de.po', $this->getPo('de'));
   +    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.fr.po', $this->getPo('fr'));
   

   I think we have more examples of \Drupal::root() vs. $this->root. What about creating a follow up: #2928710: Eliminate usages of \Drupal::root() in tests

2. +++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationTest.php
   @@ -43,31 +43,28 @@ protected function setUp() {
   +    // Place a custom local translation in the translations directory.
   +    mkdir(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations', 0777, TRUE);
   +    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.de.po', $this->getPo('de'));
   ...
   -    // Place a custom local translation in the translations directory.
   -    mkdir(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations', 0777, TRUE);
   -    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.de.po', $this->getPo('de'));
   

   Can I be naive and ask why this moving is needed in the first place?

Just some observations.

1. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -0,0 +1,349 @@
   +    $this->setupBaseUrl();
   ...
   +    $this->prepareEnvironment();
   ...
   +    $this->setupBaseUrl();
   ...
   +    $this->prepareEnvironment();
   

   Why are we doing this twice?

2. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -0,0 +1,349 @@
   +    // Note that WebTestBase::installParameters() returns form input values
   ...
   +    // @see WebTestBase::translatePostValues()
   

   The comment needs an update.

3. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -0,0 +1,349 @@
   +    // BrowserTestBase::drupalPostForm() is to actually build the POST data string
   

   > 80 char

4. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -0,0 +1,349 @@
   +   * WebTestBase::refreshVariables() tries to operate on persistent storage,
   

   Outdated comment.

5. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -0,0 +1,349 @@
   +    $edit = $this->translatePostValues($this->parameters['forms']['install_configure_form']);
   

   Do we need this BTB as well?

6. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -0,0 +1,349 @@
   +   * Transforms a nested array into a flat array suitable for BrowserTestBase::drupalPostForm().
   

   > 80 char.

nm #62.5. interdiff looks good to me so RTBC unless bot disagrees.

That's even better.

Note that [2939753] actually deprecates ConfigAfterInstallerTestBase into a more generic ProfileTestBase. I just saw this issue now, otherwise would have coordinated earlier. Don't want to stop this going in, though, as that one is not yet RTBC. So just a heads up, maybe someone has thoughts on this...

Thanks, for the update.

+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
@@ -0,0 +1,318 @@
+require_once __DIR__ . '/../../../../includes/bootstrap.inc';

Hmmmm... why? At the very least this needs a comment. I can comment it out and run \Drupal\FunctionalTests\Installer\StandardInstallerTest successfully so it's definitely not needed for every test.

If I run tests via run-tests.sh for some reason tests based on the new InstallerTestBase don't look right...

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

Tests to be run:
  - \Drupal\FunctionalTests\Installer\StandardInstallerTest

Test run started:
  Wednesday, February 7, 2018 - 22:51

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


Test run duration: 40 sec

versus for a regular BTB test...

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

Tests to be run:
  - \Drupal\FunctionalTests\Breadcrumb\Breadcrumb404Test

Test run started:
  Wednesday, February 7, 2018 - 22:52

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

Drupal\FunctionalTests\Breadcrumb\Breadcrumb404Test            1 passes

Test run duration: 4 sec

For some reason the test summary is missing.

1. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -542,7 +542,7 @@ protected function installParameters() {
   -    global $base_url;
   +    global $base_url, $base_path;
   
   @@ -556,7 +556,7 @@ protected function setupBaseUrl() {
   -    $path = isset($parsed_url['path']) ? rtrim(rtrim($parsed_url['path']), '/') : '';
   +    $base_path = $path = isset($parsed_url['path']) ? rtrim(rtrim($parsed_url['path']), '/') : '';
   

   We've made it this far without adding globals to our functional tests, and we shouldn't do it now.

2. +++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationQueryTest.php
   @@ -43,22 +43,19 @@ protected function setUp() {
   +    $path = $this->root . DIRECTORY_SEPARATOR . $this->siteDirectory . '/profiles/mydistro';
   ...
   +    mkdir(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations', 0777, TRUE);
   +    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.de.po', $this->getPo('de'));
   +    file_put_contents(\Drupal::root() . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.fr.po', $this->getPo('fr'));
   

   Normalize on $this->root.

3. StandardInstallerTest shows up as a risky test, so apparently it's not making any assertions.

Also, you can't run --group Installer unless you've already installed a site in place. Try it:

$ cd root/drupal
$ sudo git clean -fdx sites/
$ SIMPLETEST_BASE_URL=http://your.url ./vendor/bin/phpunit -c core/ --testsuite functional --group Installer

That's an argument in favor of converting these tests to kernel tests instead of functional tests, but that might be out of scope here.

I added bootstrap.inc in #4 because many tests failed without predefined path constants (and I was afraid that they might lack something else too). But after numerous adjustments from Lendude, the tests work like a charm without it. Cool!

This is indeed some nice improvement!

Great catches by @Mile23 and @alexpott!

Everything looks great here. I think we should open a follow-up (or search for an existing issue) to fix how run-tests.sh deals with risky tests. We might have some atm and we would not know.

One thing that gives me pause about committing this now is that this would represent a massive divergence in 8.5.x and 8.6.x testing and it some of our most important tests - ie. the installer tests. I'm going to tag this needs release manager review and see if we can get consensus to backport this to 8.5.1.

+++ b/core/tests/Drupal/FunctionalTests/Installer/ConfigAfterInstallerTestBase.php
@@ -0,0 +1,44 @@
+    $default_install_path = 'core/profiles/' . $this->profile . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;

Would it make sense to make this configurable so that distributions could use this test as well?

I am thinking about ThunderInstallerTest that currently extends InstallerTestBase.

https://cgit.drupalcode.org/thunder/tree/src/Tests/Installer/ThunderInst...

@chr.fritsch I think that that might valid but in a separate issue.

Committed 21dd263 and pushed to 8.6.x. Thanks!

Can we get an 8.5.x version of the patch without the deprecation (thanks @catch for the idea). So we can keep 8.5.x and 8.6.x in sync. We'll commit once 8.5.0 is out.

I think we should open a follow-up (or search for an existing issue) to fix how run-tests.sh deals with risky tests. We might have some atm and we would not know.

We already have #2905007: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests. Do we still need a dedicated issue for risky tests?

@jibran it's not the same as a skipped test - so I would say yes. Obviously the issues are related though so should be linked.

I'll update the scope for #2905007: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests because they're different categories but the same solution.

Committed 578b093 and pushed to 8.5.x. Thanks!