Convert web tests to browser tests for quickedit module

Needs alternatives for getAjaxPageStatePostData() and drupalPost().

Initial patch... couldn't finish all test cases in Drupal\Tests\quickedit\FunctionalJavascript\QuickEditLoadingTest. Updated IS with details.

@vijaycs85++

just a quick reroll, nothing further added

Fun with concurrent sessions in testConcurrentEdit! This works locally, curious to see how the bot handles it.
We might need to move the getSession() version to BTB so we handle concurrent sessions out of the box better.

Green except for testCustomPipeline, I don't get what that is doing.

Thanks for working on this! 🙏

What don’t you understand about that last failing test?

@Wim Leers, thanks for keeping an eye on it.

If I'm reading the test right, its goal is to test the else in \Drupal\quickedit\QuickEditController::renderField. It uses quickedit_test_quickedit_render_field and a mocked URL to do so in the WebTestBase version.

So, could this just be a kernel test? That won't give us coverage of the specially constructed URL that triggers this, so it feel like we might be losing some coverage then. So probably not.

If we want to test the URL handling, we would need some way to trigger this, other then a mocked URL. What I don't see, is how you can make QuickEdit use the specially crafted URL that causes the hook to trigger and not mock it. There are no implementations of this in core, so is there anywhere else that I can steal an example from?
So basically I need a fully functional test setup to use the hook for this, not the half-mock setup we have now. Any ideas?

Lacking a proper way to test the URL, here is a straight conversion of the mocked URL coverage to BTB.

Well, that does the job I guess, bit of a clean up.

1. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
   @@ -0,0 +1,205 @@
   +    $quickedit_field_locator = '[name="field_quickedit_testing_tags[target_id]"]';
   +    $assert->waitForElementVisible('css', $quickedit_field_locator)->focus();
   +    $tags = $assert->waitForElementVisible('css', $quickedit_field_locator)->getValue();
   

   This is a bit weird that it waits for the same element multiple time, or is this just me?

2. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,388 @@
   +    // Make sure there is no "Quick edit" link.
   +    $assert->linkNotExists('Quick edit');
   

   That comment seems quite pointless

3. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,388 @@
   +    // Reload the page and check for updated body.
   +    $this->drupalGet('node/' . $nid);
   +    $assert->pageTextContains($body_text);
   ...
   +
   +    // Reload the page and check for updated title.
   +    $this->drupalGet('node/' . $nid);
   +    $assert->pageTextContains($text_new);
   

   How about checking also on the page that quick edit updates the body text properly?

4. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,388 @@
   +    // Switch to body field.
   +    $page->find('css', '[data-quickedit-field-id="node/' . $nid . '/title/en/full"]')->click();
   +    $assert->assertWaitOnAjaxRequest();
   ...
   +    // Wait and update body field.
   +    $field_locator = '.field--name-title';
   +    $text_new = 'Obligatory question';
   +    $assert->waitForElementVisible('css', $field_locator)->setValue($text_new);
   

   This documentation talks about the title field

5. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,388 @@
   +  /**
   +   * Tests that Quick Edit doesn't make fields rendered with display options
   +   * editable.
   +   */
   +  public function testDisplayOptions() {
   +    $node = Node::load('1');
   +    $display_settings = [
   +      'label' => 'inline',
   +    ];
   +    $build = $node->body->view($display_settings);
   +    $output = \Drupal::service('renderer')->renderRoot($build);
   +    $this->assertFalse(strpos($output, 'data-quickedit-field-id'), 'data-quickedit-field-id attribute not added when rendering field using dynamic display options.');
   +  }
   

   Could we open up a follow up which moves this into a kernel test?

6. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,388 @@
   +    $this->drupalLogin($this->editorUser);
   +    $this->drupalGet('node/1/edit');
   +    $image = $this->drupalGetTestFiles('image')[0];
   +    $image_path = $this->container->get('file_system')->realpath($image->uri);
   +    $page->attachFileToField('files[field_image_0]', $image_path);
   +    $alt_field = $assert->waitForField('field_image[0][alt]');
   +    $this->assertNotEmpty($alt_field);
   +    $this->drupalPostForm(NULL, [
   +      'field_image[0][alt]' => 'Vivamus aliquet elit',
   +    ], t('Save'));
   

   It's amazing that this works.

@dawehner thanks for the review.

#21.1 fixed
#21.2 removed
#21.3 That test method is specifically for the title field 'Tests the loading of Quick Edit for the title base field.', so lets keep it specific, since this has coverage in testUserPermissions
#21.4 heh, c/p left over, fixed
#21.5 yeah, it's a pretty weak test anyway, with only a negative assertion, tried a quick conversion to a kernel test and it passed even without the $display_settings passed (the WebDriver version fails as expected), so yeah a followup makes sense #3021406: Convert \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditLoadingTest::testDisplayOptions to a kernel test
#21.6 agreed!

Thanks, overall patch looks good. Just some minor observations.

1. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,387 @@
   +    $assert->waitForElementVisible('css', $field_locator)->setValue($text_new);
   ...
   +    $this->assertSession()->waitForElementVisible('css', '.quickedit-toolgroup.ops [type="submit"][aria-hidden="false"]')->click();
   

   We already have the local variable. Let's use that instead.

2. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
   @@ -0,0 +1,387 @@
   +  public function getSession($name = NULL) {
   +    // We need to pass a name to work with concurrent sessions.
   +    $name = $name ?: $this->mink->getDefaultSessionName();
   +    return $this->mink->getSession($name);
   +  }
   

   I don't see a reason to override this method. Call can get the default session name and pass it.

I don't see a reason to override this method. Call can get the default session name and pass it.

Good point! Explicit code is nicer than implicit.

Addressed #23.

Thank you @jibran
My points from #21 got addressed

1. +++ /dev/null
   @@ -1,215 +0,0 @@
   -      // Load the form again, which should now get it back from
   -      // PrivateTempStore.
   -      $quickedit_uri = 'quickedit/form/node/' . $this->node->id() . '/' . $this->fieldName . '/' . $this->node->language()->getId() . '/full';
   -      $post = ['nocssjs' => 'true'] + $this->getAjaxPageStatePostData();
   -      $response = $this->drupalPost($quickedit_uri, '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
   -      $ajax_commands = Json::decode($response);
   

   This test of PrivateTempStore usage has seemly gone missing. I'm not sure how we simulate what this is testing in a JS environment though.

2. +++ /dev/null
   @@ -1,607 +0,0 @@
   -    // Quick Edit's JavaScript would never hit these endpoints if the metadata
   -    // was empty as above, but we need to make sure that malicious users aren't
   -    // able to use any of the other endpoints either.
   -    $post = ['editors[0]' => 'form'] + $this->getAjaxPageStatePostData();
   -    $response = $this->drupalPost('quickedit/attachments', '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
   -    $message = Json::encode(['message' => "The 'access in-place editing' permission is required."]);
   -    $this->assertIdentical($message, $response);
   -    $this->assertResponse(403);
   -    $post = ['nocssjs' => 'true'] + $this->getAjaxPageStatePostData();
   -    $response = $this->drupalPost('quickedit/form/' . 'node/1/body/en/full', '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
   -    $this->assertIdentical($message, $response);
   -    $this->assertResponse(403);
   -    $edit = [];
   -    $edit['form_id'] = 'quickedit_field_form';
   -    $edit['form_token'] = 'xIOzMjuc-PULKsRn_KxFn7xzNk5Bx7XKXLfQfw1qOnA';
   -    $edit['form_build_id'] = 'form-kVmovBpyX-SJfTT5kY0pjTV35TV-znor--a64dEnMR8';
   -    $edit['body[0][summary]'] = '';
   -    $edit['body[0][value]'] = '<p>Malicious content.</p>';
   -    $edit['body[0][format]'] = 'filtered_html';
   -    $edit['op'] = t('Save');
   -    $response = $this->drupalPost('quickedit/form/' . 'node/1/body/en/full', '', $edit, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
   -    $this->assertIdentical($message, $response);
   -    $this->assertResponse(403);
   -    $post = ['nocssjs' => 'true'];
   -    $response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
   -    $this->assertIdentical(Json::encode(['message' => "The 'access in-place editing' permission is required."]), $response);
   -    $this->assertResponse(403);
   

   Hmmm where has the test coverage for this gone? This security testing is important. I think we need a separate BrowserTestBase to test the security of these endpoints.

3. +++ /dev/null
   @@ -1,607 +0,0 @@
   -      // Ensure the text on the original node did not change yet.
   -      $this->drupalGet('node/1');
   -      $this->assertText('How are you?');
   

   This part of the test seems to have gone missing. I guess in a JS environment we never have the separate requests that the old fake test had so maybe dropping this is okay (like point 1) but maybe we should implement a BTB to add back this test coverage and discuss whether it is worth it in a separate issue.

4. +++ /dev/null
   @@ -1,607 +0,0 @@
   -      // Ensure no new revision was created and the log message is unchanged.
   -      $node = Node::load(1);
   -      $vids = \Drupal::entityManager()->getStorage('node')->revisionIds($node);
   -      $this->assertIdentical(1, count($vids), 'The node has only one revision.');
   -      $this->assertIdentical($original_log, $node->revision_log->value, 'The revision log message is unchanged.');
   

   Missing too. Although I guess you could argue this is testing Node behaviour and not quickedit. But they way quickedit interacts with revisions is important so I would have expected this coverage to be maintained.

5. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditCustomPipelineTest.php
   @@ -0,0 +1,102 @@
   +      'cookies' => $this->getSessionCookies(),
   

   So the problem with doing this is that we lose the ability to debug inside the test request because the xdebug cookie is not set. Maybe there's a better way of adding the xdebug cookie than the way we currently do that would mean we don't have to think about this. Let's open a follow-up to try to address this. Because this is not the only test using getSessionCookies and then makes undebuggable requests. Like I think we can add a handler in \Drupal\Tests\BrowserTestBase::initMink() to do this.

I opened #3029750: Add default cookies in tests using a Guzzle handler to address #27.5

This is just a reroll after #3021406: Convert \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditLoadingTest::testDisplayOptions to a kernel test landed.

Hmmm #29 is one unrelated fail, and one related fail, but the related fail passes locally. Lets keep an eye on that, make sure we don't introduce new random fails.

This addresses some of the feedback from #27

#27.1 Yeah, since we don't do partial/manual requests anymore, this is impossible to test in a real javascript scenario. I would expect this to be covered in existing kernel/unit test coverage, but a quick search didn't reveal anything
#27.2 added a dedicated test for this
#27.3 yeah, see 1
#27.4 Added it back in
#27.5 The follow up issue covers this nicely, and would work with these tests staying as they are, so no change for that now.

Bleh, quick cleanup

Setting it back to RTBC after #27 has been addressed.

Unrelated fails

+++ /dev/null
@@ -1,593 +0,0 @@
-    $post = ['editors[0]' => 'form'] + $this->getAjaxPageStatePostData();
-    $response = $this->drupalPost('quickedit/attachments', '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
-    $message = Json::encode(['message' => "The 'access in-place editing' permission is required."]);
-    $this->assertIdentical($message, $response);
-    $this->assertResponse(403);
-    $post = ['nocssjs' => 'true'] + $this->getAjaxPageStatePostData();
-    $response = $this->drupalPost('quickedit/form/' . 'node/1/body/en/full', '', $post, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
-    $this->assertIdentical($message, $response);
-    $this->assertResponse(403);
-    $edit = [];
-    $edit['form_id'] = 'quickedit_field_form';
-    $edit['form_token'] = 'xIOzMjuc-PULKsRn_KxFn7xzNk5Bx7XKXLfQfw1qOnA';
-    $edit['form_build_id'] = 'form-kVmovBpyX-SJfTT5kY0pjTV35TV-znor--a64dEnMR8';
-    $edit['body[0][summary]'] = '';
-    $edit['body[0][value]'] = '<p>Malicious content.</p>';
-    $edit['body[0][format]'] = 'filtered_html';
-    $edit['op'] = t('Save');
-    $response = $this->drupalPost('quickedit/form/' . 'node/1/body/en/full', '', $edit, ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
-    $this->assertIdentical($message, $response);
-    $this->assertResponse(403);
...
-    $response = $this->drupalPostWithFormat('quickedit/metadata', 'json', $post);
-    $this->assertResponse(200);
-    $expected = [
-      'node/1/body/en/full' => [
-        'label' => 'Body',
-        'access' => TRUE,
-        'editor' => 'form',
-      ],
-    ];
-    $this->assertIdentical(Json::decode($response), $expected, 'The metadata HTTP request answers with the correct JSON response.');
-    // Restore drupalSettings to build the next requests; simpletest wipes them
-    // after a JSON response.
-    $this->drupalSettings = $htmlPageDrupalSettings;

note to self: this was moved to a new test see #31

Fixed on commit

diff --git a/core/modules/quickedit/tests/src/Functional/QuickEditCustomPipelineTest.php b/core/modules/quickedit/tests/src/Functional/QuickEditCustomPipelineTest.php
index 30c056a2b1..00d7c201ba 100644
--- a/core/modules/quickedit/tests/src/Functional/QuickEditCustomPipelineTest.php
+++ b/core/modules/quickedit/tests/src/Functional/QuickEditCustomPipelineTest.php
@@ -36,7 +36,7 @@ public function testCustomPipeline() {
       'access content',
       'create article content',
       'edit any article content',
-      'access in-place editing'
+      'access in-place editing',
     ]);
     $this->drupalLogin($editor_user);

diff --git a/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
index 1469fd3e19..434259111c 100644
--- a/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
+++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
@@ -154,7 +154,7 @@ public function testAutocompleteQuickEdit() {
     // Click by "Quick edit".
     $this->clickContextualLink('[data-quickedit-entity-id="node/' . $this->node->id() . '"]', 'Quick edit');
     // Switch to body field.
-    $page->find('css', '[data-quickedit-field-id="node/' . $this->node->id() .  '/' . $this->fieldName . '/' . $this->node->language()->getId() . '/full"]')->click();
+    $page->find('css', '[data-quickedit-field-id="node/' . $this->node->id() . '/' . $this->fieldName . '/' . $this->node->language()->getId() . '/full"]')->click();

     // Open quickeditor.
     $quickedit_field_locator = '[name="field_quickedit_testing_tags[target_id]"]';

Committed d8c9748 and pushed to 8.7.x. Thanks!

Doesn't apply cleanly to 8.6, so putting back to needs work for a re-roll

I still need to spend probably two hours reviewing (and manually comparing) this in detail to ensure no test coverage gets lost — it's hard to do because there is so much change. I'm sure it's all for the better though — the existing tests are super hard to read and understand too due to them being written in a time that predates @dataProvider and JS tests :) Thanks so much for having pushed it this far already!

Here's an initial review.

1. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditCustomPipelineTest.php
   @@ -0,0 +1,102 @@
   + * Tests using a custom pipeline with quick edit.
   

   s/quick edit/Quick Edit/

2. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +   * Tests that Quick Edit endpoints are protected from anonymous requests.
   ...
   +  public function testEndPoints() {
   

   The description isn't reflected in the method name.

3. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +    // was empty as above, but we need to make sure that malicious users aren't
   

   "empty as above" no longer makes sense.

4. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +    $this->checkAccessIsBlocked($url, $post);
   ...
   +    $this->checkAccessIsBlocked($url, $post);
   ...
   +    $this->checkAccessIsBlocked($url, $edit);
   ...
   +    $this->checkAccessIsBlocked($url, $post);
   

   We're testing 4 different cases. Let's use @dataProvider for this? (That didn't exist back when these tests were originally written, and they make tests so much clearer!)

5. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +    $this->drupalCreateContentType([
   +      'type' => 'article',
   +      'name' => 'Article',
   +    ]);
   

   Let's move this to ::setUp(), which is possible now thanks to this having a test class of its own.

6. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +   * @param $edit
   +   *   The payload to send with the request.
   

   "edit" vs "payload". How about just "body"?

7. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +  protected function checkAccessIsBlocked($url, $edit) {
   

   This should be named assertSomething(). Wouldn't assertAccessDeniedResponse() be more accurate?

8. +++ b/core/modules/quickedit/tests/src/Functional/QuickEditEndPointsTest.php
   @@ -0,0 +1,88 @@
   +      'body' => http_build_query($edit),
   +      'query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax'],
   +      'cookies' => $this->getSessionCookies(),
   +      'headers' => [
   ...
   +      'http_errors' => FALSE,
   

   If we're converting this to use Guzzle, then let's also use the constants: RequestOptions::BODY, RequestOptions::HEADERS, etc.

9. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
   @@ -0,0 +1,207 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   @inheritdoc

10. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
    @@ -0,0 +1,207 @@
    +    // Create the vocabulary for the tag field.
    

    This comment can be removed, it's stating the obvious :)

11. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
    @@ -0,0 +1,207 @@
    +    $this->editorUser = $this->drupalCreateUser([
    +      'access content',
    +      'create article content',
    +      'edit any article content',
    +      'administer nodes',
    +      'access contextual links',
    +      'access in-place editing',
    +      ]);
    

    Indentation of last line is wrong.

12. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditAutocompleteTermTest.php
    @@ -0,0 +1,207 @@
    +    // Open quickeditor.
    

    s/quickeditor/Quick Edit/

13. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
    @@ -0,0 +1,388 @@
    +    $this->assertSame(1, count($vids), 'The node has only one revision.');
    ...
    +    $this->assertSame(1, count($vids), 'The node has only one revision.');
    

    $this->assertCount()

14. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
    @@ -0,0 +1,388 @@
    +   * Test quickedit does not appear for entities with pending revisions.
    

    s/quickedit/Quick Edit/

@Wim leers, thanks for the review, but this was already committed to 8.7.x, do we want to revert this?

I rolled it back since the review posted by @Wim Leers has some stuff that is better solved here than trying to piece it out to followups.

It sounds for me like the perfect usecase of a follow up. You can iterate for ages on tests :)

1. Fixed
2. Fixed
3. Fixed
4. BTBs are not good candidates for @dataProvider - no need to install Drupal so many times.
5. Fixed
6. Fixed
7. Fixed
8. Personally I find the strings more readable but whatevs.
9. Fixed
10. Fixed
11. Fixed
12. Fixed
13. Fixed
14. Fixed

All these changes are relatively minor in the scheme of things setting straight back to rtbc so we can be closer to being WTB free for 8.7.0.

+1 RTBC

I didn't mean to trigger a revert — I reviewed this this morning with the issue I had open (flaky internet connection, they're fixing it later today), so I missed the commit.

I spotted lots of small things during a first scan, but I primarily un-RTBC'd because I wanted to verify that no test coverage was lost. Given @alexpott confidently re-RTBC'd already, I'm totally fine with not spending an hour or two manually comparing the before vs after. :)

@Wim Leers - I agree with the your wish to not lose test coverage. I did a line-by-line review in #27 focussing on test coverage that had been removed. I should have posted a more general comment saying that I consider the resolution of #27 to result in no loss of test coverage (that I can spot). And yes reviewing this patch for that is hard.

Thanks all! Adjusting credits now.

#47: Ah, excellent, I didn't realize #27 dug that deep. Wonderful, thanks! 👏 👍

Committed to 8.7.x again :) Would need a reroll for 8.6.x. (Conflicts on QuickEditLoadingTest).

Now that #3021406: Convert \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditLoadingTest::testDisplayOptions to a kernel test is backported this applies to 8.6.x. Re-uploading to trigger a test run. If this is green we can cheery-pick to 8.6.x by doing
git cherry-pick -x a974536

Yay, thanks!

I did a line-by-line review in #27 focussing on test coverage that had been removed

For what it's worth, so did I because it was clear the permissions tests were a lot smaller than their predecessors (replaced with the stand alone test).

Look like it has some problems with SQLite:
- https://www.drupal.org/pift-ci-job/1212297
- https://www.drupal.org/pift-ci-job/1211477
- https://www.drupal.org/pift-ci-job/1212495
- https://www.drupal.org/pift-ci-job/1212502

1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditLoadingTest::testWithPendingRevision
Drupal\Core\Entity\EntityStorageException: SQLSTATE[HY000]: General error: 5 database is locked: INSERT INTO {node_revision} (nid, langcode, revision_uid, revision_timestamp, revision_log, revision_default) VALUES (?, ?, ?, ?, ?, ?); Array
(
    [0] => 1
    [1] => en
    [2] => 0
    [3] => 1551197144
    [4] => Iow`>&4t
    [5] => 0
)


/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:823
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:394
/var/www/html/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php:198

Caused by
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 5 database is locked: INSERT INTO {node_revision} (nid, langcode, revision_uid, revision_timestamp, revision_log, revision_default) VALUES (?, ?, ?, ?, ?, ?); Array
(
    [0] => 1
    [1] => en
    [2] => 0
    [3] => 1551197144
    [4] => Iow`>&4t
    [5] => 0
)


/var/www/html/core/lib/Drupal/Core/Database/Connection.php:692
/var/www/html/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php:351
/var/www/html/core/lib/Drupal/Core/Database/Connection.php:656
/var/www/html/core/lib/Drupal/Core/Database/Query/Insert.php:89
/var/www/html/core/lib/Drupal/Core/Database/Driver/sqlite/Insert.php:21
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1154
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:927
/var/www/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:657
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:448
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:814
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:394
/var/www/html/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php:198

Caused by
PDOException: SQLSTATE[HY000]: General error: 5 database is locked

/var/www/html/core/lib/Drupal/Core/Database/StatementPrefetch.php:163
/var/www/html/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php:90
/var/www/html/core/lib/Drupal/Core/Database/Connection.php:631
/var/www/html/core/lib/Drupal/Core/Database/Query/Insert.php:89
/var/www/html/core/lib/Drupal/Core/Database/Driver/sqlite/Insert.php:21
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1154
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:927
/var/www/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:657
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:448
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:814
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:394
/var/www/html/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php:198

Yep, reverted. I'll queue some SQLite runs again to illustrate the fail.

First requeue passed, adding some more with PHP 5.5 and 5.6 since 3 of the 4 @tacituseu pointed to were using that. Lets see...

I can reproduce the exact same fail by doing sudo -u _www php ./core/scripts/run-tests.sh --verbose --concurrency 3 --repeat 30 --sqlite /tmp/coretest.sqlite --dburl sqlite://localhost/sites/default/files/db.sqlite --color --non-html --url http://drupal8alt.test/ --class 'Drupal\\Tests\\quickedit\\FunctionalJavascript\\QuickEditLoadingTest' and renaming all the tests to _test except testWithPendingRevision()

Here's a fix. The database lock occurs because of quickedit's ajax request to get additional metadata.

#62: impressive detective work! 🕵️‍♂️ Makes perfect sense.

RTBC+1, maybe third time will be a charm.

+++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditLoadingTest.php
@@ -192,6 +192,10 @@ public function testWithPendingRevision() {
+    // database locks on SQLite.

Wow!

Committed cd03cd4 and pushed to 8.7.x. Thanks!

c/p as 5b4b9d51b5 and pushed to 8.6.x