Closed (fixed)
Project:
Drupal core
Version:
10.0.x-dev
Component:
database system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
21 Oct 2022 at 18:07 UTC
Updated:
17 Nov 2022 at 09:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #3
mondrakeComment #4
mondrakeComment #5
mondrakeComment #6
mondrakeAlso reviewed the logic in
DriverSpecificKernelTestBase::setUpto skip tests that do not match the SUT's driver before database init, this allows to save some time.Comment #7
mondrakeThose failures need addressing.
Comment #8
mondrakeComment #9
daffie commentedFor the unresolved threads on the MR.
Comment #10
mondrakeThanks @daffie, points addressed.
Comment #11
daffie commentedThe added comments look good to me.
All test changes look good to me.
The IS is very clear.
For me it is RTBC.
Comment #12
catchCouple of questions on the MR.
Comment #13
mondrakeThanks for reviews. Addressed @catch's comments. Resetting to RTBC since changes are doc only.
I suggest to address the move of the test statement monitoring classes in a follow-up. Maybe a better solution would be to work with the Log class instead, instead of overriding the Connection one. I think that deserves more discussion.
Comment #14
mondrakeLet's try switching to use the standard logger here directly.
Comment #15
mondrakeActually, getting rid of the
database_statement_monitoring_testseems to simplify this test and make it more accurate since we can take log of the insert statements too, that the current solution can't since it only logs call toquery.Comment #16
mondrakeSome more work for pgsql and sqlite needed.
Comment #17
mondrakeFurther optimized EndOfTransactionQueriesTest
Comment #18
daffie commentedComment #19
daffie commentedAll code changes look good to me.
Back to RTBC.
Comment #20
mondrakeSee also https://drupal.slack.com/archives/C1BMUQ9U6/p1667249303417589 on Slack for some background.
Comment #21
mondrakeBack to NW to refactor EndOfTransactionQueriesTest
Comment #22
mondrakeFor review again.
Comment #23
daffie commentedAs requested by @catch is the test EndOfTransactionQueriesTest no longer extending the class DriverSpecificKernelTestBase. It is back to extending the class KernelTestBase.
Back to RTBC.
Comment #24
catchCouple more questions sorry:
None of the core drivers overrride TransactionTest, but the issue summary suggests that MySQL should (and that Oracle must). Is it just that MySQL doesn't need to override it after all (but that Oracle still does)? Or am I missing something?
SyntaxTest - I assume this is per-driver just for Oracle?
Nice to see the test module gone, that's a lot cleaner and glad I asked!
Comment #25
mondrakeNot MySql (the core driver based on PDO). A MySql driver based on the mysqli extension should, since mysqli does not fail when rolling back and no transaction active. See https://github.com/mondrake/mysqli/blob/main/tests/src/Kernel/mysqli/Tra... for an attempted mysqli driver.
Exactly. See https://github.com/mondrake/drudbal/blob/master/tests/src/Kernel/dbal/Sy... for example.
Comment #27
catchCommitted/pushed to 10.1.x, thanks!
Comment #28
mondrakeGreat, thanks. This could be backportable if we omit the deprecation of the test module.
Comment #30
mondrakeFleshed and published the CR.
Comment #31
mondrakeComment #32
mondrakeComment #33
mondrakeA 10.0.x and 9.5.x patch.
Comment #34
mondrakeSo we cannot backport to 9.5 unless we backport the parent first. Not sure worth the effort.
Comment #35
daffie commentedThe patch from comment #33 can be backported to 10.0.x.
Back to RTBC.
Comment #36
alexpottCommitted 92c09c8 and pushed to 10.0.x. Thanks!