Problem

  1. PIFR uses Drush to install a parent site for running tests + to enable the Simpletest module.

  2. PIFR only has to do that, because the run-tests.sh script requires an existing parent site + simpletest.module to exist and be installed.

  3. This causes a circular dependency for a whole bunch of major D8 core patches, because they cannot be committed

    1. unless Drush has been adjusted
    2. latest Drush has been manually deployed to all testbots
    3. even though it is nonsensical for Drush to implement/support upcoming changes in Drupal core to begin with

    → Deadlock.

Objective

  1. Enable run-tests.sh to (also) operate without a pre-installed Drupal site.

Proposed solution

  1. run-tests.sh gets two new (optional) parameters:

    1. --sqlite: Specifies a pathname for a SQLite database to use for the test runner (Simpletest).

      This database file is created automatically, and the Simpletest module schema is automatically created in it.

    2. --dburl: Specifies the database connection info to use within actual tests.

      This is needed, because otherwise, all tests would actually run against the SQLite database of the test runner, instead of e.g. MySQL.

  2. If there is a working Drupal installation with Simpletest module already, then run-tests.sh continues to function like previously.

    I.e., it uses the database connection info from settings.php and no extra parameters need to be specified.

Example

  1. To run tests without a pre-installed Drupal site, using a SQLite database file in /tmp and using MySQL with username 'drupal' and database drupal8 on localhost for actual tests:

    php core/scripts/run-tests.sh --sqlite /tmp/test.sqlite --dburl mysql://drupal:drupal@localhost/drupal8

Blocked issues

  1. #2176621: Remove global $databases
  2. #2210197: Remove public access to Extension::$type, ::$name, ::$filename
  3. #340723: Make modules and installation profiles only require .info.yml files
  4. #1351352: Distribution installation profiles are no longer able to override the early installer screens
  5. #1067408: Themes do not have an installation status
CommentFileSizeAuthor
#34 interdiff.txt1.2 KBsun
#34 test.sqlite.34.patch27.82 KBsun
#33 test.sqlite.33.patch27.76 KBsun
#32 interdiff.txt6.23 KBsun
#32 test.sqlite.32.patch28.83 KBsun
#29 interdiff.txt6.46 KBsun
#29 test.sqlite.29.patch26.65 KBsun
#28 interdiff.txt5.97 KBsun
#28 test.sqlite.28.patch24.11 KBsun
#24 interdiff.txt1.61 KBsun
#24 test.sqlite.24.patch22.41 KBsun
#22 interdiff.txt13.11 KBsun
#22 test.sqlite.22.patch21.09 KBsun
#20 interdiff.txt12.09 KBsun
#20 test.sqlite.20.patch49.61 KBsun
#17 interdiff.txt6.73 KBsun
#17 test.sqlite.17.patch43.24 KBsun
#15 interdiff.txt7.06 KBsun
#15 test.sqlite.15.patch43.6 KBsun
#10 test.sqlite.10.patch69.01 KBsun
#10 interdiff.txt5.27 KBsun
#8 test.sqlite.8.patch65.28 KBsun
#6 test.sqlite.6.patch68.88 KBsun
#6 interdiff.txt47.5 KBsun
#2 test.sqlite.2.patch25.03 KBsun
#2 interdiff.txt13.05 KBsun
test.sqlite.0.patch12.4 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
interdiff.txt13.05 KB
test.sqlite.2.patch25.03 KB

Incorporated #1810990: Teared down test environment leaks into parent site/test runner
Incorporated #1774388: Unit Tests Remastered™
Incorporated #1436684: Remove static cache in drupal_valid_test_ua()
Fixed tearDown() methods assume fully-installed Drupal environment.
Fixed run-tests.sh bootstrap/shutdown.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

Created: #1812908: Add php5-sqlite package on testbots

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Needs work

#1810990: Teared down test environment leaks into parent site/test runner just got committed, so this'll need a re-roll.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
interdiff.txt47.5 KB
test.sqlite.6.patch68.88 KB

Fixed _simpletest_format_summary_line() calls into t()/format_plural().
Fixed reported list of tests to execute is always empty.
Incorporated #1601146: Allow custom assertion messages using predefined placeholders
Incorporated #1770902: Theme of parent site executing test leaks into all tests
Replaced simpletest_log_read() with TestBase::parseErrorLog().

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.sqlite.8.patch65.28 KB

Merged HEAD.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
interdiff.txt5.27 KB
test.sqlite.10.patch69.01 KB

Re-implemented --clean.
Fixed test result summary always outputs number of debug messages even if zero.

Fixed fatal errors caused by PHP shutdown functions leaking from test environment into test runner.

^^ Our exception handling and usage of shutdown functions in D8 has become totally nuts.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Version: 8.x-dev » 9.x-dev
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 9.x-dev » 8.x-dev
Priority: Major » Normal
Issue summary: View changes
Issue tags: +API addition
Related issues: +#2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Priority: Normal » Major
Status: Needs work » Postponed
Issue tags: -API addition

This appears to be the best solution for #2206501: Remove dependency on Drush from test reviews, which blocks a whole bunch of major issues right now.

I restarted work on this and was happy to see that pretty much all of the necessary cleanups elsewhere throughout the system were have been performed through child/sister issues already. This means that a patch for this issue should most likely have to touch run-tests.sh only now.

However, in order to do this, we need a proper kernel that is able to operate without an existing Drupal system. That is essentially what the (very messy) code in install_begin_request() is doing, but that code cannot be re-used currently, so I started to work on that first:

#2213357: Use a proper kernel in early installer

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Postponed » Needs review
FileSize
test.sqlite.15.patch43.6 KB
interdiff.txt7.06 KB

Combined patch. Interdiff is against #2213357: Use a proper kernel in early installer

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.sqlite.17.patch43.24 KB
interdiff.txt6.73 KB

Sorry, until #2213357: Use a proper kernel in early installer has landed, I'm not able to provide interdiffs between patches here.

The interdiffs attached here are the "actual patches" for this issue; i.e., sans the changes of aforementioned issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Created #2214055: Programmed form submission does not get a triggering_element

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.sqlite.20.patch49.61 KB
interdiff.txt12.09 KB
  1. Allow to pass a --db parameter to run-tests.sh (PIFR).
  2. Do not use default error/exception handler in run-tests.sh.
  3. Fixed ContextualUnitTest.
  4. Fix UX of test class assertion summaries, in order to identify all failing tests.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.sqlite.22.patch21.09 KB
interdiff.txt13.11 KB

Correction: We need (1) a test runner db (2) a default db, and (3) a test db derived from (2).

The apparently reversed signature of Database::getConnection() gave me the most delightful DX and brain-melts in this patch...

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.sqlite.24.patch22.41 KB
interdiff.txt1.61 KB

Fixed $GLOBALS['conf'] leaks into all tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
Priority: Major » Critical
Parent issue: » #2206501: Remove dependency on Drush from test reviews
Related issues: +#1812908: Add php5-sqlite package on testbots, +#2213357: Use a proper kernel in early installer, +#1810990: Teared down test environment leaks into parent site/test runner, +#1770902: Theme of parent site executing test leaks into all tests, +#1799310: WebTestBase->setUp() broken for sqlite, +#1813286: _simpletest_format_summary_line() should not use t(), +#2214055: Programmed form submission does not get a triggering_element

Updated issue summary to reflect the current situation.

Given how many major issues are currently blocked on the Drush dependency in testbot reviews, I think it's fair to say that this is critical.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
test.sqlite.28.patch24.11 KB
interdiff.txt5.97 KB
  1. Fixed database connection key 'test' is too ambiguous in tests; rename to 'test-runner'.
  2. Fixed SimpleTestTest: global $conf may not necessarily be defined.
  3. Fixed run-tests.sh docs of --sqlite and --url.

This patch is expected to come back green.

Please note that the latest implementation is designed to work both with and without a preexisting "parent site". That's the reason for why this patch actually passes against testbots again.

The solution actually has to be validated in combination with the patch for PIFR in #2206501: Remove dependency on Drush from test reviews — which makes PIFR skip the entire installation of Drupal + Simpletest module prior to running tests for D8.

However, that's yet another hairy deadlock situation → The plan is to get this patch into core independently of the PIFR change, because patching both core and PIFR on a testbot at the same time is... complex.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.sqlite.29.patch26.65 KB
interdiff.txt6.46 KB

Polished and completed documentation.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
Issue tags: -DIE

Wondering what I can do to move forward here? — Pretty much all of my major core issues are blocked on this issue.

The change to FormBuilder is already split into #2214055: Programmed form submission does not get a triggering_element + slightly modified there to hopefully be RTBC and committed ASAP.

Further clarified the issue summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
  1. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
@@ -13,6 +13,9 @@
+
+  public static $modules = array('contextual');

    Insert usual documentation nitpick here ;) 

  2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -992,6 +1010,8 @@ private function prepareEnvironment() {
     $this->originalConfig = $GLOBALS['config'];
+    // @todo Remove all remnants of $GLOBALS['conf'].
+    $this->originalConf = isset($GLOBALS['conf']) ? $GLOBALS['conf'] : NULL;

    Reference https://drupal.org/node/2183323 here? You mentioned that's not all of them, but references usually help to remember to clean up code or at least try to do so when fixing those issues.

  3. If you try to run the applied patch without an installed D8 and without --sqlite, it spits out a few php notices and then an exception. Would be nice to handle this a bit better and say that you need to use --sqlite/--dburl to run tests without an existing installation?
  4. The improvement to the output is very nice!

  5. When running with --sqlite/--dburl, the test is executed, but at the end it throws an exception in simpletest_clean_results_table(), there seems to be another db_query() that should be updated.

  6. Noticed that some tests throw exceptions ( try running the "Entity API" group, at least one of them was due to "RuntimeException: Missing $settings['hash_salt'] in settings.php.
    in drupal_get_hash_salt()". This was Drupal\system\Tests\Entity\EntityAccessTest, might be more reasons than that?

    Seems like we'd need to run the tests before committing this? I can do that once you fixed this, shouldn't take too long.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.sqlite.32.patch28.83 KB
interdiff.txt6.23 KB

Thanks for reviewing and testing, @Berdir! Attached patch fixes all of the issues you encountered.

  1. Added phpDoc to ContextualUnitTest::$modules.
  2. Added @see to @todo in TestBase.
  3. Fixed missing default database connection error handling.
  4. Added validation for test runner database (schema).
  5. Updated simpletest_clean_results_table() to use test runner database connection.
  6. Fixed required Settings do not exist when executed without settings.php.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.sqlite.33.patch27.76 KB

#2214055: Programmed form submission does not get a triggering_element has landed, so this is ready to go now.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.sqlite.34.patch27.82 KB
interdiff.txt1.2 KB

Polished simpletest_script_reporter_display_summary().

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Since we currently experience many random test failures, I've split that global $conf fix into a separate issue:

#2217087: global $conf leaks into all tests

We should also commit the following ASAP:

#2209461: Settings from test runner leak into all tests

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

I reviewed this patch and it looks great, IMO.

If we do a reroll, I think that +require_once __DIR__ . '/../vendor/autoload.php'; in run-tests.sh could use require. That line is not new to this patch so RTBC. sun confirmed that this can go in now or can be rerolled quickly if #2217087: global $conf leaks into all tests gets in first. I vote for just committing this ASAP.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: test.sqlite.34.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

34: test.sqlite.34.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community

Testbot fluke.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

I'll confess that most of this is over my head, but I don't think it's particularly under any other core maintianers' heads, so. :) Apart from the contextual bug fix, this is all changes in the underlying test system, so shouldnt affect too many other peoples' work. I'm really curious how on earth that contextual unit test is passing now, though.

This gets us some nice flexibility to be able to make adjustments to the test runner without forcing jthorson and rfay to jump through hoops though, so let's give it a shot.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

John_B’s picture

John_B CreditAttribution: John_B commented

I have added a tutorial to help noobs get this working locally, in a new section of the documentation at https://drupal.org/node/645286, section "Running tests without an installed Drupal site (D8)". A quick check by someone better qualified may be useful.