UncaughtExceptionTest is still failing for SQLite and Postgres. Fix it.

CommentFileSizeAuthor
#8 6-8-interdiff.txt1.05 KBalexpott
#8 2533946.8.patch3.87 KBalexpott
#6 2533946.6.patch3.71 KBalexpott
#6 1-6-interdiff.txt1.49 KBalexpott
#1 2533946.1.patch3.71 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
Issue tags: +PostgreSQL, +sqlite
FileSize
2533946.1.patch3.71 KB

The patch attached passes on mysql, postgres and sqlite.

The whole removing exception assertions was bogus because the overridden error method was stopping these... we were just removing the assertRaw() assertions... lol.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I've run this test successful on Postgres, SQLite and MySQL - I think we're good to go here.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

RTBC - Nice catch. Testing the tests and watching the watchers, uhm monkeys is not easy ...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented 
+++ b/core/modules/system/src/Tests/System/UncaughtExceptionTest.php
@@ -153,49 +150,35 @@ public function testExceptionContainer() {
+    switch ($this->container->get('database')->driver()) {
+      case 'mysql':
...
+      default:
+        // We can not carry out this test.
+        $this->pass('Unable to run \Drupal\system\Tests\System\UncaughtExceptionTest::testLostDatabaseConnection for this database type.');
+        return;
+    }

Do we mind explaining why sqlite doesn't work?

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

Not sure if the pgsql tests are a pgsql issue or if this is fixed for pgsql: https://dispatcher.drupalci.org/job/default/631/testReport/System/Drupal...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review
FileSize
1-6-interdiff.txt1.49 KB
2533946.6.patch3.71 KB

@Mixologic yep that indicates a difference between my Postgres 9.4 and DrupalCI's 9.1. Let's try and make the test not db version dependent.

Status: Needs review » Needs work

The last submitted patch, 6: 2533946.6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2533946.8.patch3.87 KB
6-8-interdiff.txt1.05 KB

re #4/@dawehner sqlite does not use user credentials.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Can't we just change the path or so? Then it will fail on missing tables or so on the first query, resulting in essentially the same problem. We don't care about the specific error (I think, maybe assertion messages do?) just a scenario where the database is not reachable/broken.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@Berdir that doesn't work because it breaks the parent site as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

On DrupalCI neither Postgres not SQLite fail due to UncaughtException test. I think this is as good as we can do.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

The sqlite issue - I'm wrong the parent site is not broken... investigating.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Okay it is really hard to test this on SQLite - it does not crash at the same point as MySQL and Postgres - they crash in the page cache middleware. Whereas SQLite crashes during routing which means that HttpKernel::handle() eats the exception because $catch is set to TRUE.

I think we should open a separate issue to handle SQLite testing for this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

+1 to separate issue.

The reason mysql and postgresql already crash early is due to the injected database connection through the cache backend. But I guess the user is logged in then we never actually execute a query there.

This is a problem with the way the weird database service works (it's still a static factory and designed to open the database connection when the object is requested, not when we execute the first query. That's because it's still the 7.x design hidden there which assumes you get the connection because you're going to immediately execute a query. With injection, that's no longer the case.

So I *think* that you could get the same early exception in sqlite if you don't have a logged in user/session and page cache tries to check if there is a cached page.

But again, +1 to separate issue, we can experiment there.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

So the reason we don't fail at the same place for SQLite is...

    try {
      $result = $this->connection->query('SELECT cid, data, created, expire, serialized, tags, checksum FROM {' . $this->connection->escapeTable($this->bin) . '} WHERE cid IN ( :cids[] ) ORDER BY cid', array(':cids[]' => array_keys($cid_mapping)));
    }
    catch (\Exception $e) {
      // Nothing to do.
    }

in Cache::getMultiple() - this is done because there are situations where the table might not exist and then is created on demand.

We should definitely handle SQLite in a separate issue. Created #2534818: Support SQLite in UncaughtException::testLostDatabaseConnection

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Looks good to me :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 3634822 on 8.0.x 
    Issue #2533946 by alexpott: UncaughtExceptionTest is a monkey in the...

Status: Fixed » Closed (fixed)

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