Problem/Motivation

The FunctionalJavaScript tests for the Media module frequently fail on SQLite with:

SQLSTATE[HY000]: General error: 5 database is locked

This seems to occur primarily in three tests

  1. Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testErrorMessages()
  2. Drupal\Tests\media\FunctionalJavascript\MediaStandardProfileTest::testMediaSources()
  3. Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testErrorMessages()<

There are records of such fails at least as far back as January, but they went from being a 1% fail rate to a 50% or more fail rate when the Drupal test environments were updated at the beginning of March for the Node 16 and Chromedriver updates.

It has also been shown that these failures occur even if the Media JavaScript test group is run by itself (without other tests), and that reducing concurrency to 1 is not sufficient to solve the problem. (See #3273599: [testing] Try reducing concurrency of JS tests to see if that would fix SQLite.)

Proposed resolution

  1. Skip these three test methods on SQLite until a fix is found. See #15.
  2. Figure out why this is happening and make the tests work consistently with SQLite.

Remaining tasks

  1. Look into why these test fail.
  2. Unskip them with a fix.
  3. Verify that the DB locking woes are gone on SQLite.
  4. Reviews / refinements.
  5. RTBC

User interface changes

N/A

API changes

TBD

Data model changes

? Maybe depending on the issue.

Release notes snippet

TBD

CommentFileSizeAuthor
#36 3273626-36.patch7.31 KBkim.pepper
#34 interdiff-32_34.txt1.07 KBrakesh.regar
#34 3273626-34.patch7.31 KBrakesh.regar
#32 3273626-32.patch5.27 KBquietone
#32 diff-27-32.txt3.4 KBquietone
#27 core-3273626-27.patch7.11 KBnod_
#24 core-sqlite-mediaprofile-fail-3273626-24.patch5.04 KBnod_
#23 core-sqlite-mediaprofile-fail-3273626-23.patch4.67 KBnod_

Issue fork drupal-3273626

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented
Issue summary: View changes
Parent issue: » #2829040: [meta] Known intermittent, random, and environment-specific test failures
Related issues: +#3273599: [testing] Try reducing concurrency of JS tests to see if that would fix SQLite, +#1120020: SQLite database locking errors cause fatal errors

 

xjm opened merge request !2057

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented
Issue summary: View changes
Status: Active » Needs review

 

Spokje made their first commit to this issue’s fork.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented

Thanks @Spokje!

We don't need multiple runs agains multiple databases, just SQLite databases.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented

The use statements made me realize we're actually doing the wrong thing -- we don't need to do a version compare or the use statements; we want to skip it regardless of why on SQLite. Should have read the snippet I copied more closely.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented
Assigned: Unassigned » xjm
Status: Needs review » Needs work

Hmm, both databases have the same # of passes for the tests, so something's still not right. Going to debug this locally.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented
Assigned: xjm » Unassigned
Status: Needs work » Needs review

The three fails are actual existing random failures, not the database dying.

I also confirmed locally that the tests are actually skipped on SQLite now by running one of the affected tests.

I think this is ready.

dww made their first commit to this issue’s fork.

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

The test results are indeed improved, as expected. I closely reviewed the MR code. Everything looks good. Agreed with the latest commits to skip these tests on SQLite regardless of version. Makes the code easier to read, and seems safer given the intention/scope of this issue.

It seems slightly weird to use this same issue nid for both marking these skipped, and for the @todo target for getting them working again. I'd have assumed those should be separate issues for scope management. But since @xjm did it this way in the first place, and they're the ultimate arbitrator of scope, I defer. 😉

The MR claimed it was blocked, and needed a rebase. I just did that, but since I didn't change any code, I still feel ok RTBC'ing this. At least for phase 1, this is ready.

Thanks!
-Derek

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented

Lots of unrelated layout_builder fails. 😢 Requeued.

  • larowlan committed 4990c41 on 10.0.x 
    Issue #3273626 by xjm, dww, Spokje: Drupal Media JavaScript test suite...

  • larowlan committed 70a2bfd on 9.4.x 
    Issue #3273626 by xjm, dww, Spokje: Drupal Media JavaScript test suite...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Title: Drupal Media JavaScript test suite causes database locks on SQLite » Resolve issue with tests for Drupal Media JavaScript causing database locks on SQLite
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Merged the MR to 9.4 and 10.0, technically we should have a separate URL for the todo, as well as comply with the todo format, but because we're talking about stuff that is disrupting actual work, I've committed this without those changes.

Re-purposing this issue to restore the tests and resolve the underlying issue.

Dropping the priority accordingly.

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Issue summary: View changes

Sweet, thanks! Updating summary accordingly.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented

Over in #3241653: Occassional test failures with SQLite: SQLSTATE[HY000]: General error: 5 database is locked the maintainers of FarmOS seem to have some success battling the same problem by running SQLite3 tests in sequence instead of in parallel.
Note: They run their tests on GitHub

I _think_ #3273599: [testing] Try reducing concurrency of JS tests to see if that would fix SQLite where @xjm ran the tests with no concurrency is (slightly) different from running tests in sequence?
Anyway: Just clutching at straws.

Taran2L made their first commit to this issue’s fork.

Taran2L opened merge request !2066

Taran2L’s picture

Taran2L
Primary language Ukrainian
Location Lviv
CreditAttribution: Taran2L at Lemberg Solutions commented

hm, interesting ... testing this idea: #1120020-96: SQLite database locking errors cause fatal errors

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented

This seemed to help https://www.drupal.org/project/drupal/issues/2855596#comment-14472477

Taran2L’s picture

Taran2L
Primary language Ukrainian
Location Lviv
CreditAttribution: Taran2L at Lemberg Solutions commented

Okay, seems like solution with a timeout does not work ... or am I applying it incorrectly?

@_nod solution from other issue looks much better

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented
FileSize
core-sqlite-mediaprofile-fail-3273626-23.patch4.67 KB

This seem to fix the testMediaSources failures.

got the same problems with some nightwatch tests a while back, i was pretty sure the click method didn't wait for the form to finish submitting, meaning we'd do the form save and node save at the same time. if we wait for the form save to finish it should fix it.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented
FileSize
core-sqlite-mediaprofile-fail-3273626-24.patch5.04 KB

added a comment as to why it's necessary to wait.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented

adding a little delay before the node save happens seems to work for the remaining failures in the testErrorMessages: https://www.drupal.org/project/drupal/issues/2855596#comment-14473027

  • xjm committed 261aa81 on 9.3.x authored by larowlan 
    Issue #3273626 by xjm, dww, Spokje: Drupal Media JavaScript test suite...
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented
Status: Active » Needs review
FileSize
core-3273626-27.patch7.11 KB

Seems like it does the trick, 50x run here #2855596-38: [ignore] nod_ test isssue

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Acquia commented

Failures are unrelated to the tests in question it seems

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented

One of them is extremely rare, so I am going to re-queue.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#2031261: Make SQLite faster by combining multiple inserts and updates in a single query
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
FileSize
diff-27-32.txt3.4 KB
3273626-32.patch5.27 KB

This is just a reroll. Patch also applies to 10.0.x and 10.1.x.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

To me adding a sleep feels like working around the problem.

If this needed, can we at least make that sleep only happen when we're actually using SQLite, now we're "punishing" everyone.

rakesh.regar’s picture

rakesh.regar
Rakesh Hindoniya
Primary language English
Location Rajasthan, 🇮🇳
CreditAttribution: rakesh.regar as a volunteer and at Specbee commented
FileSize
3273626-34.patch7.31 KB
interdiff-32_34.txt1.07 KB

Added check for SQLite

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper at PreviousNext commented
FileSize
3273626-36.patch7.31 KB

Re-roll of #34

Status: Needs review » Needs work

The last submitted patch, 36: 3273626-36.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.