Problem/Motivation

Steps to reproduce:

  1. Enable testing module (simpletest)
  2. Go to the UI and select 6 or 7 unit tests (regular functional tests are OK)
  3. Press "Run tests"

Expected output

The tests will be executed and I see the results

Actual output

Uncaught PHP exception:

Uncaught PHP Exception Drupal\Core\Database\DatabaseExceptionWrapper:
"SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'test_class' at row 1: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => Drupal\Tests\commerce\Unit\Resolver\ChainCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\ChainLocaleResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultLocaleResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\ChainStoreResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\DefaultStoreResolverTest
    [:db_insert_placeholder_2] => fail
    [:db_insert_placeholder_3] => PHPunit Test failed to complete
    [:db_insert_placeholder_4] => Other
    [:db_insert_placeholder_5] => Drupal\Tests\commerce\Unit\Resolver\ChainCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\ChainLocaleResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultCountryResolverTest,Drupal\Tests\commerce\Unit\Resolver\DefaultLocaleResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\ChainStoreResolverTest,Drupal\Tests\commerce_store\Unit\Resolver\DefaultStoreResolverTest
    [:db_insert_placeholder_6] => 0
    [:db_insert_placeholder_7] => .../sites/default/files/simpletest/phpunit-3.xml
)
" at .../core/lib/Drupal/Core/Database/Connection.php line 676, referer: http://localhost/admin/config/development/testing

Proposed resolution

Remove optimisation of trying to run PHPUnit tests before starting the batch. Now we have so many PHPUnit test types this results in timeouts causing the above error.
Remove is_subclass_of() from SimpletestTestForm::submitForm() - this could potentially autoload all tests in the same request which just does not work.

Remaining tasks

User interface changes

None

API changes

None - a BC layer is added to simpletest_run_tests()

Data model changes

None

CommentFileSizeAuthor
#73 2575725-73.patch7.99 KBalexpott
#73 63-73-interdiff.txt1.24 KBalexpott
#69 2575725-69.patch13.36 KBalexpott
#69 63-69-interdiff.txt1.24 KBalexpott
#65 uncaught-php-exception-2575725-65.patch7.96 KBpashupathi nath gajawada
#63 2575725-63.patch7.96 KBalexpott
#63 59-63-interdiff.txt2.2 KBalexpott
#59 2575725-59.patch6.55 KBalexpott
#54 five views unit tests.png345.51 KBjonathan1055
#38 views_ui_unit_tests_with dummy_@group.png55.86 KBjonathan1055
#38 views_ui_three_unit tests.png71.51 KBjonathan1055
#36 interdiff-36.txt4.68 KBTorenware
#36 2575725-36.patch9.48 KBTorenware
#31 interdiff-31.txt2.87 KBTorenware
#31 2575725-31.patch4.8 KBTorenware
#26 2575725-25.patch2.51 KBrajeshwari10
#22 2575725-22.patch2.14 KBrajeshwari10
#19 2575725-19.patch2.14 KBrajeshwari10
#16 2575725-16-interdiff.txt536 byteslokapujya
#16 2575725-16.patch1.42 KBlokapujya
#15 2575725-15-interdiff.txt1.08 KBlokapujya
#15 2575725-15.patch1.12 KBlokapujya
#13 2575725-13.patch1.05 KBlokapujya
#7 2575725-7-use_text_for_class_and_func.patch1.35 KBandrewbelcher
#5 2575725-use_text_for_class_and_func.patch1.35 KBandrewbelcher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

iMiksu created an issue. See original summary.

alauzon’s picture

It also happened to me only when I was running the test with UI in HTTPS mode. In HTTP mode it was not doing it.

dawehner’s picture

Does the same thing happens if you run things via the command line? I'm just curious here.

alauzon’s picture

It's worse. On the command-line I've got a way more Exceptions than with the UI. I did not have time to check why it was that way so I went the UI way.

andrewbelcher’s picture

Status: Active » Needs review
FileSize
1.35 KB

This is because test_class is VARCHAR(255). With namespaced test classes, that really isn't going to hold many tests! I found it just running the Page Manager/Panels tests (16 tests). The same issue exists with function...

Changing them to TEXT (which also involved settings a size on the reporter index's use of test_class seems to resolve this for me. I've attached a patch, but I don't know really know what's happening well enough to know if changing to TEXT (from varchar_ascii which is a special machine name case) or changing the key length is going to have other consequences - so definitely needs a review from someone who does know!

Status: Needs review » Needs work

The last submitted patch, 5: 2575725-use_text_for_class_and_func.patch, failed testing.

andrewbelcher’s picture

Status: Needs work » Needs review
FileSize
1.35 KB

Oops, think that was just using TEXT rather than text...

Status: Needs review » Needs work

The last submitted patch, 7: 2575725-7-use_text_for_class_and_func.patch, failed testing.

hedrickbt’s picture

retracting my comment.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Torenware’s picture

Saw this today with 8.2.x.

Both the test_class and function fields can take a comma delimited list of test_classes , and of function, which will be even longer.

This field really does need to be a TEXT type field; varchar(255) just won't work with namespaced classes.

I got this, BTW, just by doing the following:

  1. Open the Testing page
  2. Find the line for "simpletest" and check the checkbox.
  3. Click Run Tests
  4. AND NOW WE GONNA DIE.
Torenware’s picture

@dawehner in #3: this is specific to the Testing form page. run-test.sh does NOT have this issue.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.05 KB

Lets try using the text field, without changing the index. I don't know why the index needs to change.

I couldn't test this locally, so trying it out here.

Status: Needs review » Needs work

The last submitted patch, 13: 2575725-13.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
1.08 KB

According to the test errors, text can't have a default value.

lokapujya’s picture

A text index Needs to have a prefix length. All tests are passing so I guess that Drupal assigns a default length. I'm going to in-revert the index length, so that we are explicitly setting a length.

albertski’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good and it ran perfectly for me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This needs an update path for the schema changes so sites which have simpletest installed are fixed.

rajeshwari10’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

Added hook_update.

Please Review.

Thanks!!

Status: Needs review » Needs work

The last submitted patch, 19: 2575725-19.patch, failed testing.

albertski’s picture

Can you update the function description to follow Drupal Coding standards:

/**
 * Implements hook_update().
 */

Can you also double check that it should 8102. It may be correct I'm just not sure if it should just be 8100.

rajeshwari10’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

@albertski ,

changed the function description as per coding standard.

for 8100 and 8101 it was not showing any updates.

So, I used 8102 for which it was working.

Thanks!!

Status: Needs review » Needs work

The last submitted patch, 22: 2575725-22.patch, failed testing.

Torenware’s picture

+++ b/core/modules/simpletest/simpletest.install
@@ -180,3 +176,21 @@ function simpletest_uninstall() {
+function simpletest_update_8102() {
+  db_change_field('simpletest', 'test_class', 'test_class', array(
+    'type' => 'text',
+    'not null' => TRUE,
+    'description' => 'The name of the class that created this message.',
+  ));
+  db_change_field('simpletest', 'function', 'function', array(
+    'type' => 'text',
+    'not null' => TRUE,
+    'description' => 'Name of the assertion function or method that created this message.',
+  ));
+}
 

I think we're doing the wrong thing here. simpletest.install declares hook_schema(), so we need to do something more like https://www.drupal.org/node/2535384. Note db_change_field() is now deprecated.

dawehner’s picture

Well, we can indeed replace db_change_field() by Database::getConnection()->schema()->changeField()

rajeshwari10’s picture

Assigned: Unassigned » rajeshwari10
Status: Needs work » Needs review
FileSize
2.51 KB

I removed db_change_field. and followed the steps given in the link in #24.

Thanks!!

Status: Needs review » Needs work

The last submitted patch, 26: 2575725-25.patch, failed testing.

rajeshwari10’s picture

I am not getting why the tests are failing.

Please can anyone guide.

thanks!!

Torenware’s picture

Let me see what we can see here. I did the simplest possible test here: I just applied your patch and ran "drush updb". Here's what I get on a fresh install with simpletest enabled:

vagrant@itest:/var/www/drupalvm/drupal$ drush updb
The following updates are pending:

simpletest module :
  8102 -   Implements hook_update_N()  Changing type to 'text'.  Removing the default value.

Do you wish to run all pending updates? (y/n): y
 [error]  SQLSTATE[42000]: Syntax error or access violation: 1170 BLOB/TEXT column 'test_class' used in key specification without a key length: ALTER TABLE {simpletest} CHANGE `test_class` `test_class` TEXT NOT NULL COMMENT 'The name of the class that created this message.'; Array
(
)

 [notice] Performing simpletest_update_8102
 [error]  Failed: SQLSTATE[42000]: Syntax error or access violation: 1170 BLOB/TEXT column 'test_class' used in key specification without a key length: ALTER TABLE {simpletest} CHANGE `test_class` `test_class` TEXT NOT NULL COMMENT 'The name of the class that created this message.'; Array
(
)

 [notice] Cache rebuild complete.
 [notice] Finished performing updates.

This looks to be something @lokapujya found in #16. I'll see if this is in fact the case.

Torenware’s picture

Looks like our problem relates to the index on ('test_class', 'message_id'), which needs to be dropped before we can modify the field. The update executes w/o error. I'm going to see if this makes the rest of the errors go away as well.

We still need a test of the update, once this works. We may also need to do a little tweaking for Postqres compatibility IIRC.

Torenware’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
2.87 KB

Patch implementing the index drop and re-add. Again, not complete w/o test, but this does run locally both from an install and from an update. Letting the testbot get a taste.

jonathan1055’s picture

I had this problem when running several tests from the Rules module, but with Devel, and also on Scheduler (which I maintain), I can run many more tests without getting the problem, and without any patch.

In the failed sql insert, all the classes were comma-concatenated into one long string which is inserted into the simpletest table at the start of the test run. With Rules, selecting the first five tests in one run is ok, as the total length is 243 (although it seems to fail with "no results to show"), but picking the first six causes the insert error as this makes 288 chars. But with Devel and Scheduler, only one test class is inserted into the table at a time, when each class starts a row gets added (which is subsequently deleted when that test completes) then the next test class is added. Thus we get no failures and I can run all 16 test classes in one run, each with an average length of 50 chars making 800 chars in total.

So why do we get the differing behaviour? I expect there is a simple solution - be we need to understand it, because just increasing the test_class field length may not be the proper solution if it is meant to only hold one class at a time, as 255 would be enough for that.

Jonathan

Torenware’s picture

In the failed sql insert, all the classes were comma-concatenated into one long string which is inserted into the simpletest table at the start of the test run.

Correct. This SQL gets generated when using the admin/config/testing page.

So why do we get the differing behaviour? I expect there is a simple solution - be we need to understand it, because just increasing the test_class field may not be the proper solution if it it meant to only hold one class at a time as 255 would be enough.

Not sure why. I'm assuming that you're using admin/config/testing in all these cases. I'd guess that you're using the interface slightly differently, and therefore getting a different code path in the submitForm() handler in its form class. But if you read the code, I don't see why changing the module is relevant in and of itself; the processing code does not even really "know" what module is posting the tests to the DB.

Torenware’s picture

A question about creating the appropriate test here.

The relevant manual page seems to be https://www.drupal.org/node/2536494. Part of the process is to create a DB-neutral dump file, and run the installer against it:

  /**
   * {@inheritdoc}
   */
  protected function setDatabaseDumpFiles() {
    $this->databaseDumpFiles = [
      // This path will work for a Core update test that is living in /core/modules/foo/src/Tests/Update.
      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',
      __DIR__ . 'PATH TO YOUR PHP SETUP FILE',
    ];
  }

  /**
   * Tests that DESCRIBE YOUR UPDATE HERE.
   */
  public function testUpdateHookN() {
    // Run the updates.
    $this->runUpdates();

   // Now load your data and test it.
  }

Looking in Core, I see that there are a variety of "DBND"s in core/modules/system/test/fixtures, but no where else. I assume that system/tests/fixtures/update/drupal-8.bare.standard.php.gz is a "moving target"; its content should change over the dev cycle. For the test to work, I need to use a dump that reflects the state of things before my schema change.

So, I'd guess I should use the standard database neutral dump. But at my patch level, if I enable the simpletest module, note that simpletest_schema() will have run according to my new state and will reflect the update level subsequent to the update level I need to test. So I need a state of the schema that's not defined in either the base file (because simpletest is not in the standard profile) and not in the patch's simpletest.install level.

The manual page does not cover this case. I could, of course, create my own neutral dump that reflects the old simpletest.install, and simpletest being pre-enabled. But this is about 135K worth of fixture to check in.

What's the recommended thing to do here?

Torenware’s picture

RE #34. The manual does not make this clear, but it sounds like the test needs to:

  • Inherit from Drupal\system\Tests\Update\UpdatePathTestBase
  • DO NOT enable simpletest (???? is this even possible if we are running a test?)
  • Add the bare base to the list of dumps.
  • Add another dump that simulates what the simpletest table looks like before we run the update (i.e., build the table appropriately, and set an arbitrary update level for the module).

This is bit strange, but am I getting this right here? This is an unusually "meta" kind of test.

Torenware’s picture

Test has been added. The test works, in that it verifies that the update actually ran, and that the module version actually increased. I'm not sure if pulling the schema and examining it actually verifies that we made the changes we thought we made (i.e., the fields are now text rather than varchar_ascii), since if I could do a PDO-equivalent of MySQL's "desc", that strikes me as more probative. Any suggestions on that score?

andrewbelcher’s picture

If there isn't an abstraction level means of checking the changes have taken effect, we could always try inserting something with length > 255 which would throw an exception if it were too long (ie hadn't changed).

jonathan1055’s picture

Just done some more research into my questioning in #33

Not sure why. I'm assuming that you're using admin/config/testing in all these cases. I'd guess that you're using the interface slightly differently, and therefore getting a different code path in the submitForm() handler in its form class.

@Torenware - I am using admin/config/testing UI in exactly the same way for both cases. I have discovered what make the difference. Normal indivudual simpletests run one at a time and you can execute as many as you want with no limit. It is only when the tests are PHPunit tests when we get the problem, as this is the scenario where the individual test classes are comma-concatenated. I am still unsure if this is exactly what is intended to be done with this table. If you select a small number of unit tests (for example in core Views_UI pick the first three test, which are unit tests) views_ui three tests
then the SQL exception is not thrown because we have

$classname => Drupal\Tests\views_ui\Unit\Form\Ajax\RearrangeFilterTest,Drupal\Tests\views_ui\Unit\ViewListBuilderTest,Drupal\Tests\views_ui\Unit\ViewUIObjectTest

and the combined length is 148. However, a new exception is thrown by the line in

$reflection = new \ReflectionClass($classname); // fails if $classname is two or more comma-separated classes.

and we get

ReflectionException: Class Drupal\Tests\views_ui\Unit\Form\Ajax\RearrangeFilterTest,Drupal\Tests\views_ui\Unit\ViewListBuilderTest,Drupal\Tests\views_ui\Unit\ViewUIObjectTest does not exist in ReflectionClass->__construct() (line 333 of core/modules/simpletest/src/TestDiscovery.php).

This does not look right, to try to use a concatenated list of classes in the new \ReflectionClass statement.

If you comment out this line, and fake the next using $doc_comment = '* @group xyz */'; so that the unit tests actual get past this point, you end up with no results, and an output:
views_ui unit test results

So we need to know exactly what the purpose of the concatenated class list is for, when inserted into the {simpletest} table, before we have confidence that just increasing the character length is the right tihing to do.

Torenware’s picture

@jonathan1055 -- it sounds like you have an understanding of how the test list gets generated, although I'm not sure if it's in scope for this particular issue. Certainly, it sounds like changing the the field definitions from varchar_ascii to text prevents a crash whichever code path you go over here.

I don't see any of the possible paths you describe as "wrong", as long as tests you intend to run get run, and as long as we don't get the spurious exception described in this issue. So do we really need to modify that code? It certainly works. And if it isn't broken, well, you know how the old expression goes.

Torenware’s picture

Also, I'm not sure from your description

However, a new exception is thrown by the line in

$reflection = new \ReflectionClass($classname); // fails if $classname is two or more comma-separated classes.
and we get

ReflectionException: Class Drupal\Tests\views_ui\Unit\Form\Ajax\RearrangeFilterTest,Drupal\Tests\views_ui\Unit\ViewListBuilderTest,Drupal\Tests\views_ui\Unit\ViewUIObjectTest does not exist in ReflectionClass->__construct() (line 333 of core/modules/simpletest/src/TestDiscovery.php).

as to how to reproduce this issue. It's not the code path we hit earlier in this issue, since we died in the database code.

Certainly, calling new \ReflectionClass() on a list of classes is wrong. But it's not the error originally reported in the issue; moreover, that code was wrong, as you do point out, even if the length of the $classname is under 255 ascii characters.

On the fairly safe assumption you can describe how to reproduce this problem, might this be better reported as either a new issue, or as a follow-up to this one?

jonathan1055’s picture

Title: Uncaught PHP Exception when selecting too many tests to be tested from UI » Uncaught PHP Exception when selecting too many PHPunit tests in UI

… as long as tests you intend to run get run, and as long as we don't get the spurious exception described in this issue. So do we really need to modify that code? It certainly works.

I was not suggesting to change any code. I was showing that PHPunit tests currently do not run and they produce more exceptions. The fake $doc_comment I had to introduce was to get around another exception where $annotations['group'] is empty. This was all to show that even when you avoid all exceptions you still get no results.

I'm not sure from your description as to how to reproduce this issue. It's not the code path we hit earlier in this issue, since we died in the database code.

As I showed in the first screen-shot, just select the first three tests in the views_ui group. This avoids the sql 255 limit exception but shows that running any PHPunit tests via the admin UI is buggy and fairly broken anyway.

Certainly, calling new \ReflectionClass() on a list of classes is wrong. But it's not the error originally reported in the issue

The purpose of going to this detail was to show that even with your change to increase the table field length, you will immediately hit this next problem.

The whole point I am making is that the original exception reported on this issue is only caused when running PHPunit tests, not for normal functional tests, and I wanted to show that PHPunit tests still fail badly anyway, when you run a small number which do not cause the original exception. Thus we might be addressing the wrong problem by solely focussing on the table field length. I would not like to see you wasting your effort in increasing the length and writing the associated update function, and a test for that function, if the whole PHPunit functionality needs to be altered. It might be that the proper fix for PHPunit tests removes the comma-concatenation and the length increase was entirely unnecessary. Then we will have added extra code and complications for no actual benefit.

alexpott’s picture

  1. +++ b/core/modules/simpletest/src/Tests/Update/UpdateSimpletestTableSchemaTest.php
    @@ -0,0 +1,37 @@
    +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',
    +      __DIR__ . '/../../../tests/fixtures/update/simpletest_emulate_old_schema.php',
    

    Is simpletest enabled in the filled standard install? If it is - then that'd be preferred to doing the emulation.

  2. +++ b/core/modules/simpletest/src/Tests/Update/UpdateSimpletestTableSchemaTest.php
    @@ -0,0 +1,37 @@
    +    $this->assertEqual(8102, drupal_get_installed_schema_version('simpletest', TRUE));
    

    This will break the moment we add another hook update N - I think the best way to test this is too run the same query before and after - before should fail and after should pass.

  3. +++ b/core/modules/simpletest/simpletest.install
    @@ -142,7 +139,7 @@ function simpletest_schema() {
    -      'reporter' => array('test_class', 'message_id'),
    +      'reporter' => array(array('test_class', 255), 'message_id'),
    

    I think this index length might need adjusting for 4byte UTF-8. I think it might have to be 191. But also how is this index actually used? It seems weird to be dumping a really long aggregated string with all the classes and then be using an index...

alexpott’s picture

  protected function getResults($test_id) {
    return $this->database->select('simpletest')
      ->fields('simpletest')
      ->condition('test_id', $test_id)
      ->orderBy('test_class')
      ->orderBy('message_id')
      ->execute()
      ->fetchAll();
  }

This query in SimpletestResultsForm would benefit from an index on test_id... this index does not help...

The query in simpletest_last_test_get() does not use the index either... again a test_id index would help...

The query in simpletest_script_load_messages_by_test_id() also does not use it ... and yet again a test_id index would help.

The query in simpletest_script_open_browser() also does not use it... and yet again a test_id index would help.

The query in simpletest_clean_results_table() also does not use it... and yet again a test_id index would help.

The query in TestBase::deleteAssert() does not use it.

And that is all I can find. Afaics we should drop the index - it is useless in core. And we should file a follow up to add an index on the test_id column.

jonathan1055’s picture

But also how is this index actually used? It seems weird to be dumping a really long aggregated string with all the classes and then be using an index...

this index does not help...

Afaics we should drop the index - it is useless in core

Excellent review and investigation @alexpott, thank you. This all lends weight to my argument in #38 and #41 that we should not be doing anything to fix/increase the 255 char limit until we know why the test classes are comma-concatenated, what purpose it serves, and whether this code will stay, as there are many other problems with running PHPunit tests via the simpletest UI.

I have found the following - and any of these might render the increase length obsolete:
#2566767: Don't allow running phpunit-based tests via the UI
#2608532: Simpletest UI munges PHPUnit-based test output
#2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate
#2643624: Fix SimpletestPhpunitRunCommandTest

rajeshwari10’s picture

Assigned: rajeshwari10 » Unassigned
Torenware’s picture

Is simpletest enabled in the filled standard install? If it is - then that'd be preferred to doing the emulation.

I'll decompress it and see. But if yeah, can do.

This will break the moment we add another hook update N - I think the best way to test this is too run the same query before and after - before should fail and after should pass.

How best can I test if the field has in fact updated? The test of update level was in any case a proxy for what I really want to do: check that the table schema changed.

The only thing I can think of is to put in string longer than 255 bytes and see if it throws. Is this the right kind of approach?

I think this index length might need adjusting for 4byte UTF-8. I think it might have to be 191. But also how is this index actually used? It seems weird to be dumping a really long aggregated string with all the classes and then be using an index...

It seems likely per your next comment that this index is not doing anything useful we know about, so the approach is to simply remove it and not create it (and add one to test_id, which may well be helpful). If we're wrong, and the index is needed for some reason, hopefully the full test suite will smoke this out.

I can't see why anything in contrib would care about this index either, so we're likely safe in doing this.

Torenware’s picture

@alexpott: I'm running into an odd problem using drupal-8.filled.standard.php.gz. Even if the only update in simpletest.install, run-tests.sh fails:


// ....
  protected function setDatabaseDumpFiles() {
    $this->databaseDumpFiles = [
      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.filled.standard.php.gz',
    ];
  }
// ....

  public function testUpdateHookN() {
    $this->runUpdates();
  }

The failure output looks like this:

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

Tests to be run:
  - Drupal\simpletest\Tests\Update\UpdateSimpletestTableSchemaTest

Test run started:
  Monday, June 13, 2016 - 20:09

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


Fatal error: Maximum function nesting level of '256' reached, aborting! in /var/www/drupalvm/drupal/core/modules/locale/src/LocaleTranslation.php on line 128

Fatal error: Maximum function nesting level of '256' reached, aborting! in Unknown on line 0
FATAL Drupal\simpletest\Tests\Update\UpdateSimpletestTableSchemaTest: test runner returned a non-zero error code (255).
PHP Fatal error:  Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 2006 MySQL server has gone away' in /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Statement.php:59
Stack trace:
#0 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Statement.php(59): PDOStatement->execute(Array)
#1 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php(610): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(81): Drupal\Core\Database\Connection->query('INSERT INTO {si...', Array, Array)
#3 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(32): Drupal\Core\Database\Driver\mysql\Connection->query('INSERT INTO {si...', Array, Array)
#4 /var/www/drupalvm/drupal/core/modules/simpletest/src/TestBase.php(484): Drupal\Core\Database\Driver\mysql\Insert->execute()
#5 /var/www/drupalvm/drupal/core/scripts/run-tests.sh(651): Drupal\simpletest\TestBase::insertAssert('4', 'Drupal\\s in /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php on line 671

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 2006 MySQL server has gone away' in /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Statement.php:59
Stack trace:
#0 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Statement.php(59): PDOStatement->execute(Array)
#1 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php(610): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(81): Drupal\Core\Database\Connection->query('INSERT INTO {si...', Array, Array)
#3 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(32): Drupal\Core\Database\Driver\mysql\Connection->query('INSERT INTO {si...', Array, Array)
#4 /var/www/drupalvm/drupal/core/modules/simpletest/src/TestBase.php(484): Drupal\Core\Database\Driver\mysql\Insert->execute()
#5 /var/www/drupalvm/drupal/core/scripts/run-tests.sh(651): Drupal\simpletest\TestBase::insertAssert('4', 'Drupal\\s in /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php on line 671

If I change the setDatabaseDumpFiles() to use the bare dump, it works:

  protected function setDatabaseDumpFiles() {
    $this->databaseDumpFiles = [
      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',      
    ];
  }

with output

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

Tests to be run:
  - Drupal\simpletest\Tests\Update\UpdateSimpletestTableSchemaTest

Test run started:
  Monday, June 13, 2016 - 20:18

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

Drupal\simpletest\Tests\Update\UpdateSimpletestTableSchemaTe 209 passes

Test run duration: 1 min

There's only one test I can find in Core that uses drupal-8.filled.standard.php.gz, which is Drupal\system\Tests\Update\UpdatePathTestBaseFilledTest. I'm not sure why it works where the trivial case won't work from me here; I tried inheriting from the same class it does, and I still get the same error.

I'm going to go back to using the bare version for now. I'm not sure if I'm doing something subtly (or unsubtly) wrong here, but I think I may have found an issue with using the filled dump.

Torenware’s picture

Status: Needs work » Needs review
FileSize
10.68 KB
3.9 KB

I've put in Alex's suggestions, with the exception of the use of the filled dump file. While the filled dump does indeed enable simpletest, I'm still not getting it to work. I've also not added an index for test_id yet. If I do: how best to test that an index has been created in updater?

I can of course add the index on test_id if y'all think this is in scope.

jonathan1055’s picture

Hi Torenware,

Just checking that you have seen my comments in #41 and #44 as you have not responded to them yet. If you want to go ahead anyway, and ignore the likely case that you are wasting a huge amount of your time on this, and adding unnecessary and complex code, that's OK with me, just say that you acknowledge you've seen my comments, and I will stop hassling you about this issue ;-)

One good thing is that I'm learning alot from your coding and commentry, so that's a nice side-effect anyway.

Jonathan

alexpott’s picture

Status: Needs review » Needs work

@jonathan1055 my guess is that the class list is used to determine what tests to run :)

In other news I can not repeat the test fail - I've selected way more than 30 tests with class names that add up to way longer than 255 characters.

Does anyone have a way to reproduce the test fail with only core tests?

alexpott’s picture

I've done the steps in #11 and nothing blew up. @Torenware are you sure it blew in the same way as reported in this issue? I can't see anywhere in the code it would insert a list of classes like this.

alexpott’s picture

function simpletest_run_phpunit_tests($test_id, array $unescaped_test_classnames, &$status = NULL) {
  $phpunit_file = simpletest_phpunit_xml_filepath($test_id);
  simpletest_phpunit_run_command($unescaped_test_classnames, $phpunit_file, $status);

  $rows = simpletest_phpunit_xml_to_rows($test_id, $phpunit_file);
  // A $status of 0 = passed test, 1 = failed test, > 1 indicates segfault
  // timeout, or other type of failure.
  if ($status > 1) {
    // Something broke during the execution of phpunit.
    // Return an error record of all failed classes.
    $rows[] = [
      'test_id' => '1',
      'test_class' => implode(",", $unescaped_test_classnames),
      'status' => 'fail',
      'message' => 'PHPunit Test failed to complete',
      'message_group' => 'Other',
      'function' => implode(",", $unescaped_test_classnames),
      'line' => '0',
      'file' => $phpunit_file,
    ];
  }
  return $rows;
}

Ah here it is - so it only happens if a PHPUnit test fails. This is pretty bad code since it is not telling you which PHPUnit test failed :(

alexpott’s picture

I've created #2748315: Fix indexes on simpletest table to fix the indexes.

I think we need to consider the special handling of phpunit - whilst it is nice running them all at once - not knowing which one failed sucks. And to have another fail that hides the failing test really really sucks.

There are moves to make the simpletest UI a command line builder rather than an actually runner which would solve this. But that does not help people in the interim. We could kludge and go the way of the current patch - or we could just remove the ability to run all the phpunit tests at once and run them one by one through the batch. Imo this is the way to go because now far longer tests are becoming PHPUnit so people might hit this just through max execution time.

jonathan1055’s picture

Issue summary: View changes
FileSize
345.51 KB

@alexpott, To answer your quesion in #50, select six unit tests in the 'views' section. Note this is "views" with lower case, not "Views" with upper case. [edited to say 'six tests', as the screenshot only had five ticked]

Ah here it is - so it only happens if a PHPUnit test fails. This is pretty bad code since it is not telling you which PHPUnit test failed :(

Yes that is it. I did all this analysis back in #38, and listed those other issues which all highlight all the problems with running PHPunit tests via the simpletest UI. That's what I have been trying to tell you all along. Concatenating the class names is not used anywhere, and also causes other problems, so it's quite likely that this part of the code will change radically or become obsolete. I see no benefit in changing the schema here and writing the update function, before those other major issues are resolved.

If you follow my repro steps in #38 you will find out just how broken the PHPunit test runs are.

We could kludge and go the way of the current patch

This patch won't solve the problem, it will just show up the next exception - as I explained in #38

Torenware’s picture

I've done the steps in #11 and nothing blew up. @Torenware are you sure it blew in the same way as reported in this issue? I can't see anywhere in the code it would insert a list of classes like this.

I was using 8.2.x when I saw this. Just tried it; still happens. I get the "The website encountered an unexpected error." white screen, with this error in the apache log:

AH01071: Got error 'PHP mes\
sage: Uncaught PHP Exception Drupal\\Core\\Database\\DatabaseExceptionWrapper: "SQLSTATE[22001]: String data, right truncated: 1406 Data too\
long for column 'test_class' at row 166: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, fil\
e) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4,\
:db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7), (:db_insert_placeholder_8, :db_insert_placeholder_9, :db_ins\
ert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_p\
laceholder_15), (:db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_plac\
eholder_20, :db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23), (:db_insert_placeholder_24, :db_insert_placeho\
lder_25, :db_insert_placeholder_26, ...\n', referer: http://itest.dev/admin/config/development/testing

I'm pulling up the Testing page, and checking off the "simpletest" module line, which selects all of the tests for that module. This suffices to trigger the problem. I'm not sure if the behavior is the same in 8.1.x, however.

Ah here it is - so it only happens if a PHPUnit test fails. This is pretty bad code since it is not telling you which PHPUnit test failed :(

I'm guessing those lines get invoked if phpunit out and out crashes -- note the fake test_id. The fails do get logged, but I think it's in simpletest_phpunit_testcase_to_row(). Which is confusing as all hell, to be sure.

So, yeah, you do know which PHPUnit tests failed. But since it's taking a lot of time to figure out how the code is doing that, yeah, there might be some issues with code ;-)

So the behavior is less broken than you thought, but it's still nothing to rejoice in.

Torenware’s picture

@jonathan1055,

I have seen your comments, but there are two significant issues with them:

  1. I can't reproduce your problem. Once I use lokapujya's approach and modify the field definitions, I can use the Simpletest Testing page again, and I don't hit any Reflection related crashes. So it does not appear to be true that the issues are directly related.
  2. While I think you've found a genuine problem with some of the Simpletest code, I don't think you've found the problem reported here. This is not a general bug on problems with the admin/config/testing page, but a specific bug people have seen and reported. So my guess is that your issue needs to either be in another issue, or potentially as a follow-up to this one. But for this issue itself, I think you're out of scope.

As we've seen, the code used to dispatch tests and show their results is very knotty, is hard to understand, and likely needs to be factored enough so that it's easy to diagnose and discover problems with it. So it's not surprising that there are multiple bugs that might surface, or that might be hidden by other bugs. But it makes sense to report the issues with good reproducible bug descriptions. You should definitely do that. But perhaps not in this particular issue number.

YMMV, but that's why I haven't referred to this since #39 and #40.

Torenware’s picture

@jonathan1055 -- OK, now I've seen it. But I can only get it to happen on a completely clean install, which is likely why I hadn't seen it up to now. If I recreate my install from scratch, and do the procedure from #11 with the patch from #48 applied, I'll get your crash. But if I rerun the procedure, no crash. Since I'm using drupalvm, rebuilding is easy, at least. But to see your crash, I need a clean database.

Yeah, we need to understand what the rationale is for putting multiple test classes on a single result object. I can certainly split out the test classes in SimpletestResultsForm::addResultForm(), which might prevent the crash. But who knows if that actually makes sense.

This looks to be an example of something I've discussed with Ryan Aslett aka Mixologic concerning the testbot. If PHPUnit gets hit with certain kinds of errors, the process just dies, and essentially nothing useful comes back to a parent process that has forked a phpunit child process. I think what we're doing here is passing the full set of phpunit tests to a master phpunit process that itself forks the listed tests as children. If one of the children blow up, it takes down the parent with it, w/o passing back any information concerning how it died; the information should be in a JUnit style XML file, which doesn't get built in this case. So our test runner only knows what it passed to the exploded phpunit processes -- which is the list of tests delegated to phpunit.

As alexpott points out, this case ends up dumping these mystery concatenated test_class values. Since you can't tell what died, they share a sort of truncated result object, so if we split out the concatenated classes, you can at least display something. But all of the listed classes will display as errors, which is not very useful.

Probably a good issue for Alex to triage and figure out what's salvageable here. From what Mixologic told me, it sounds like the problem is a known PHPUnit issue which has yet to be fixed upstream. PHPUnit just doesn't fail gracefully right now. Not sure what we can do about this.

alexpott’s picture

I think the thing to do here is to remove the PHPUnit optimisation and just run them one by one. Therefore we'll never get into concatenating classnames.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.55 KB

This removes the false optimisation for PHPUnit - now we have so much more PHPUnit testing the idea that we should run them all before the batch starts feels wrong. This patch just makes them part of the regular simpletest batch run. Yes we can argue that the simpletest UI shouldn't run phpunit tests and we should just provide a CLI command for people to run but atm we have the UI so let's make it work.

@Torenware I don't think there is anything upstream to fix - this issue occurs because of how we've integrated phpunit into the Simpletest UI - building UIs is not really PHPUnit's concern.

There's no interdiff because this is a different approach to solve the problem.

dawehner’s picture

  1. +++ b/core/modules/simpletest/simpletest.module
    @@ -122,22 +122,22 @@ function _simpletest_format_summary_line($summary) {
    -    simpletest_process_phpunit_results($phpunit_results);
    +  // We used to separate PHPUnit and Simpletest tests for a performance
    +  // optimization. In order to support backwards compatibility check if these
    +  // keys are set and create a single test list.
    +  // @todo https://www.drupal.org/node/2748967 Remove BC support in Drupal 9.
    +  if (isset($test_list['simpletest'])) {
    +    $test_list = array_merge($test_list, $test_list['simpletest']);
    +    unset($test_list['simpletest']);
    

    I'm a bit sceptical that we need this BC layer, this function is really just called from the submit function, but well, if you think its useful...

  2. +++ b/core/modules/simpletest/simpletest.module
    @@ -352,15 +394,21 @@ function _simpletest_batch_operation($test_list_init, $test_id, &$context) {
    +    \Drupal::moduleHandler()->invokeAll('test_finished', array($test->results));
    

    +1 for not executing hooks for phpunit tests.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

We can remove the BC later, if we really need to.

@alexpott
Seriously, amazing fix!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 2575725-59.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
7.96 KB

Wow the form was setting SIMPLETEST_BASE_URL environment variable. That really shouldn't be happening there. I think we should set it to the $base_url global if it is set.

I've tested this with run-tests.sh and it works fine - that doesn't set the global.

dawehner’s picture

  1. +++ b/core/modules/simpletest/simpletest.module
    @@ -304,9 +304,17 @@ function simpletest_phpunit_configuration_filepath() {
    +  // tests can browser the site under test.
    

    nitpick: tests can browse, instead of can browser

  2. +++ b/core/modules/simpletest/simpletest.module
    @@ -344,6 +352,9 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
       putenv('SIMPLETEST_DB=');
    +  if ($base_url) {
    +    putenv('SIMPLETEST_BASE_URL=' . $existing_simpletest_base_url);
    +  }
    

    Its confusing that we copy the old base url over, but don't need it for the DB.

pashupathi nath gajawada’s picture

Please find the updated the patch as suggested in #64.
Thanks,

dawehner’s picture

@pashupathi nath gajawada Please provide an interdiff next time. Thank you.

pashupathi nath gajawada’s picture

Sure @Daniel Wehner.

Torenware’s picture

@Torenware I don't think there is anything upstream to fix - this issue occurs because of how we've integrated phpunit into the Simpletest UI - building UIs is not really PHPUnit's concern.

I may be misquoting/misdescribing what Mixologic told me. This was while I was trying to figure out why my traits as used in phpunit tests were killing the test bot. But my sense is by dispatching phpunit tests directly, as you do in #59, would get around that problem even if I did understand him correctly.

alexpott’s picture

@dawehner re-resetting the environment variable to the existing value - I think I was being overly defensive here - just worrying about changing a user's currently set value. But reading the docs...

Adds setting to the server environment. The environment variable will only exist for the duration of the current request. At the end of the request the environment is restored to its original state.

And see the output of:

$ php -r "putenv('php_test=1'); print getenv('php_test');" && echo $php_test
1

@pashupathi nath gajawada thanks for trying to fix #64.1, just to note - your patch includes quite a few whitespace errors.

Patch attached is based on #63 and rewrites the comment completely that #64.1 pointed out was wrong.

Status: Needs review » Needs work

The last submitted patch, 69: 2575725-69.patch, failed testing.

dawehner’s picture

+++ b/core/modules/simpletest/simpletest.module
+++ b/core/modules/simpletest/simpletest.module
@@ -309,9 +309,10 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun

@@ -309,9 +309,10 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
   // functional tests can connect to the database.
   putenv('SIMPLETEST_DB=' . Database::getConnectionInfoAsUrl());
 
-  // Setup an environment variable containing the base URL so that functional
-  // tests can browser the site under test.
-  $existing_simpletest_base_url = getenv('SIMPLETEST_BASE_URL');
+  // Setup an environment variable containing the base URL, if it is available.
+  // This allows functional tests to browse the site under test. When running
+  // tests via CLI, core/phpunit.xml.dist or core/scripts/run-tests.sh can set
+  // this variable.
   if ($base_url) {
     putenv('SIMPLETEST_BASE_URL=' . $base_url);
   }

Ah, so this doesn't override the value defined in phpunit.xml.dist?

alexpott’s picture

@dawehner hopefully this code is never called from phpunit ;)

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
7.99 KB

Rebased patch on latest 8.2.x

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Oh yeah that's a good point.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: 2575725-73.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

  • catch committed f90b3df on 8.2.x
    Issue #2575725 by alexpott, lokapujya, rajeshwari10, Torenware,...

  • catch committed 7322caa on 8.1.x
    Issue #2575725 by alexpott, lokapujya, rajeshwari10, Torenware,...
catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/modules/simpletest/simpletest.module
@@ -224,6 +224,48 @@ function simpletest_process_phpunit_results($phpunit_results) {
+function simpletest_summarise_phpunit_result($results) {

Needs to be summarize. Fixed that on commit though.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

alexpott’s picture

Damn I'm losing my fight against the zed's :D

jonathan1055’s picture

@Torenware - thank you for seeing my point, and for all your input.
@dawehner - great reviewing
and finally ... @alexpott Awsome change! You really sorted out this problem - thank you!

jonathan1055’s picture

Title: Uncaught PHP Exception when selecting too many PHPunit tests in UI » Run PHPunit tests as regular tests to avoid exception when selecting too many PHPunit tests in UI

Changed the title to reflect the actual change made, as this issue is linked and referenced from several other places.

klausi’s picture

Title: Run PHPunit tests as regular tests to avoid exception when selecting too many PHPunit tests in UI » Uncaught PHP Exception when selecting too many PHPunit tests in UI

Hm, I don't agree with the title change.

The PHPUnit tests are our regular tests, the Simpletests are the odd ones that will all be converted and removed. See also #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase). Which reminds me to open an issue to completely remove Simpletest UI, because developers run their tests from the command line or IDE. We are not in the business of maintaining a graphical test runner, it is a waste of developer time.

klausi’s picture

jonathan1055’s picture

Title: Uncaught PHP Exception when selecting too many PHPunit tests in UI » Run PHPunit tests one at a time via UI to avoid exception when selecting too many

@klausi OK, fair enough, I used the wrong terminology. But I think the title should reflect what was actually changed, rather than simply state 'uncaught exception', particularly as the final solution was a structural change.

Just to record it explicitly here: When using the UI, the PHPunit tests are now executed one at a time, via the same batch processing and loop that runs the simpletest webtests. Function simpletest_process_phpunit_tests() is called separately for each PHPunit test class selected, instead of being called just once with all the test classes. When the tests are executed via run-tests.sh then the process is unchanged and all test classes are fed to simpletest_process_phpunit_tests() in one call.

Thanks for the info and links to the other issues. I can see that many major changes are coming for PHPunit and WebTest tests, but it is good to have this exception fixed now, before those other changes come along.

Jonathan

Status: Fixed » Closed (fixed)

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