Improve `ext/pdo_mysql` tests cleanup #11879

alexandre-daubois · 2023-08-05T10:37:03Z

Follow-up of #11855, same but for ext/pdo_mysql

javiereguiluz · 2023-08-10T13:49:54Z

This is a fantastic contribution. Thanks a lot Alex. I hope this gets merged so PHP can parallelize these tests in the CI server.

jorgsowa · 2023-10-09T14:48:17Z

This is a creative change, however I don't see the consistency in the naming.

File last_insert_id.phpt gets test_last_insert_id table name.
File pdo_mysql_exec gets test_mysql_exec table name, trimming pdo prefix.

Would be good to know what's the rule of naming tables.

@kamil-tekiela, what do you think about this PR?

Girgias

Most of my complaints are about the existings tests just being bad...

Some typos and other stuff but overall LGTM (sorry for taking this long I forgot)

Girgias · 2023-10-10T12:15:18Z

ext/pdo_mysql/tests/bug66528.phpt

-MySQLPDOTest::dropTestTable();
+$db = MySQLPDOTest::factory();
+$db->exec('DROP TABLE IF EXISTS test_66528');


It might make sense to keep this method and just pass a string to it with the table nape

Girgias · 2023-10-10T12:15:44Z

ext/pdo_mysql/tests/bug75177.phpt

+$pdo->query("CREATE TABLE test_75177 (`bit` bit(8)) ENGINE=InnoDB");
+$pdo->query("INSERT INTO test_75177 (`bit`) VALUES (1)");
+$pdo->query("INSERT INTO test_75177 (`bit`) VALUES (0b011)");
+$pdo->query("INSERT INTO test_75177 (`bit`) VALUES (0b01100)");


Insert queries could be combined into one

Girgias · 2023-10-10T12:18:42Z

ext/pdo_mysql/tests/pdo_mysql_exec_load_data.phpt

@@ -66,8 +66,8 @@ if (($row = $stmt->fetch(PDO::FETCH_ASSOC)) && ($row['value'] != '')) {

    /* affected rows related */

-    exec_and_count(2, $db, 'DROP TABLE IF EXISTS test', 0);
-    exec_and_count(3, $db, sprintf('CREATE TABLE test(id INT NOT NULL PRIMARY KEY, col1 CHAR(10)) ENGINE=%s', PDO_MYSQL_TEST_ENGINE), 0);
+    exec_and_count(2, $db, 'DROP TABLE IF EXISTS test_mysql_exec_load_data', 0);


This shouldn't be needed

Girgias · 2023-10-10T12:19:11Z

ext/pdo_mysql/tests/pdo_mysql_exec_select.phpt

-        exec_and_count(2, $db, 'DROP TABLE IF EXISTS test', 0);
-        exec_and_count(3, $db, sprintf('CREATE TABLE test(id INT NOT NULL PRIMARY KEY, col1 CHAR(10)) ENGINE=%s', PDO_MYSQL_TEST_ENGINE), 0);
-        exec_and_count(4, $db, "INSERT INTO test(id, col1) VALUES (1, 'a')", 1);
+        exec_and_count(2, $db, 'DROP TABLE IF EXISTS test_mysql_exec_select', 0);


This should not be needed anymore

Girgias · 2023-10-10T12:19:36Z

ext/pdo_mysql/tests/pdo_mysql_last_insert_id.phpt

-        $db->exec('DROP TABLE IF EXISTS test');
+        $db->exec('DROP TABLE IF EXISTS test_pdo_mysql_last_insert_id');
        if ('0' !== ($tmp = $db->lastInsertId()))
            printf("[003] Expecting '0'/string got '%s'/%s", var_export($tmp, true), gettype($tmp));


This should be dropped

Girgias · 2023-10-10T12:24:19Z

ext/pdo_mysql/tests/pdo_mysql_prepare_native.phpt

+        prepex(51, $db, 'DROP TABLE IF EXISTS test_prepare_native');
+        prepex(52, $db, 'CREATE TABLE test_prepare_native(id INT, label CHAR(255)) ENGINE=MyISAM');
+        if (is_object(prepex(53, $db, 'CREATE FULLTEXT INDEX idx1 ON test_prepare_native(label)', null, null, true))) {


Girgias · 2023-10-10T12:25:39Z

ext/pdo_mysql/tests/pdo_mysql_stmt_blobfromstream.phpt

@@ -53,11 +53,11 @@ unlink($file);
                return false;
            }

-            $db->exec('DROP TABLE IF EXISTS test');
-            $sql = sprintf('CREATE TABLE test(id INT, label BLOB) ENGINE=%s', PDO_MYSQL_TEST_ENGINE);
+            $db->exec('DROP TABLE IF EXISTS test_stmt_blobfromstream');


This should be moved between the 2 calls to this function, or maybe even split into a new test IDK.

Also the set-up class seems to be common to some tests?

Girgias · 2023-10-10T12:29:15Z

ext/pdo_mysql/tests/pdo_mysql_stmt_fetch_serialize.phpt

@@ -79,15 +79,14 @@ MySQLPDOTest::skip();
        if (0 != $db->getAttribute(PDO::MYSQL_ATTR_DIRECT_QUERY))
            printf("[002] Unable to turn off emulated prepared statements\n");

-        $db->exec('DROP TABLE IF EXISTS test');
-        $db->exec(sprintf('CREATE TABLE test(id INT, myobj BLOB) ENGINE=%s',
+        $db->exec(sprintf('CREATE TABLE test_stmt_fetchserialize(id INT, myobj BLOB) ENGINE=%s',


This is test is kinda pointless/outdated in its current form. What it probably should be is 3 different tests test sting the 3 different serialization mechanisms (or 4 if you cound the default one)

Girgias · 2023-10-10T12:31:00Z

ext/pdo_mysql/tests/pecl_bug_5200.phpt

@@ -14,17 +14,17 @@ PDOTest::skip();
 require __DIR__ . '/../../../ext/pdo/tests/pdo_test.inc';
 $db = PDOTest::test_factory(__DIR__. '/common.phpt');

-$db->exec("CREATE TABLE test (bar INT NOT NULL, phase enum('please_select', 'I', 'II', 'IIa', 'IIb', 'III', 'IV'))");
+$db->exec("CREATE TABLE test_pcl_bug_5200 (bar INT NOT NULL, phase enum('please_select', 'I', 'II', 'IIa', 'IIb', 'III', 'IV'))");


Suggested change

$db->exec("CREATE TABLE test_pcl_bug_5200 (bar INT NOT NULL, phase enum('please_select', 'I', 'II', 'IIa', 'IIb', 'III', 'IV'))");

$db->exec("CREATE TABLE test_pecl_bug_5200 (bar INT NOT NULL, phase enum('please_select', 'I', 'II', 'IIa', 'IIb', 'III', 'IV'))");

Typo

Girgias · 2023-10-10T12:31:28Z

ext/pdo_mysql/tests/pecl_bug_5780.phpt

-$db->exec("CREATE TABLE test2 (login varchar(32) NOT NULL, password varchar(64) NOT NULL)");
-$db->exec("INSERT INTO test2 (login, password) VALUES ('testing', 'testing')");
-$db->exec("INSERT INTO test2 (login, password) VALUES ('test2', 'testpw2')");
+$db->exec("CREATE TABLE test_pcl_bug_5780 (login varchar(32) NOT NULL, data varchar(64) NOT NULL)");


You typoed the spelling of PECL everywhere here

kocsismate

Thanks for your work! :)

kocsismate · 2023-10-16T20:55:14Z

ext/pdo_mysql/tests/bug75177.phpt

-$pdo->query("INSERT INTO $tbl (`bit`) VALUES (1)");
-$pdo->query("INSERT INTO $tbl (`bit`) VALUES (0b011)");
-$pdo->query("INSERT INTO $tbl (`bit`) VALUES (0b01100)");
+$pdo->query("DROP TABLE IF EXISTS test_75177");


drop not needed

kocsismate · 2023-10-16T20:55:25Z

ext/pdo_mysql/tests/bug75177.phpt

@@ -14,20 +14,20 @@ require_once(__DIR__ . DIRECTORY_SEPARATOR . 'mysql_pdo_test.inc');
 $pdo = MySQLPDOTest::factory();

 $tbl = "test";


unused variable

kocsismate · 2023-10-16T21:14:54Z

ext/pdo_mysql/tests/pdo_mysql_attr_max_buffer_size.phpt

@@ -46,7 +45,7 @@ if (MySQLPDOTest::isPDOMySQLnd())
                printf("[%03d] id = %d, val = %s... (length: %d)\n",
                    $offset, $id, substr($val, 0, 10), strlen($val));
            }
-            $db->exec('DROP TABLE IF EXISTS test');
+            $db->exec('DROP TABLE IF EXISTS test_attr_max_buffer_size');


this doesn't seem needed anymore

kocsismate · 2023-10-16T21:18:43Z

ext/pdo_mysql/tests/pdo_mysql_local_infile_directory_denied.phpt

@@ -40,14 +40,13 @@ if (!defined('PDO::MYSQL_ATTR_LOCAL_INFILE_DIRECTORY')) {
 	MySQLPDOTest::createTestTable($db, MySQLPDOTest::detect_transactional_mysql_engine($db));

 	try {
-		exec_and_count(1, $db, 'DROP TABLE IF EXISTS test', 0);


a few similar DROP TABLE statements called via the exec_and_count() function were not removed beforehand

alexandre-daubois · 2023-10-17T06:48:00Z

Thank you all for your review, I'll have a look at it soon! 😃

KentarouTakeda · 2023-11-11T01:57:18Z

@alexandre-daubois
Hi, Could you please merge alexandre-daubois#2

KentarouTakeda · 2023-11-15T11:34:22Z

@alexandre-daubois
You seem to be very busy, I'm sorry.

Looks like there are some other pull requests waiting for this to be merged. I'll wait a few days and then re-create the pull request.

alexandre-daubois · 2023-11-15T13:42:48Z

Thank you @Girgias @kocsismate, I did my best to address all your comments!

Girgias

Minor nits and then LGTM :)

ext/pdo_mysql/tests/bug80458.phpt

Girgias · 2023-11-16T00:16:52Z

ext/pdo_mysql/tests/mysql_pdo_test.inc

@@ -166,11 +166,11 @@ class MySQLPDOTest extends PDOTest {
                preg_match('/Client API version.*mysqlnd/', $tmp));
    }

-    static function dropTestTable($db = NULL) {
+    static function dropTestTable($db = NULL, $tableName = 'test') {


Maybe it makes sense to flip the param other in a follow up PR.

(Also adding types)

Good idea, I'll have a look!

About adding types, in this PR or in a follow-up alos?

Follow up PR :)

Alright I'll have a look at it!

Girgias · 2023-11-16T00:18:26Z

ext/pdo_mysql/tests/pdo_mysql_exec.phpt

@@ -178,7 +178,7 @@ MySQLPDOTest::skip();
 require __DIR__ . '/mysql_pdo_test.inc';
 $db = MySQLPDOTest::factory();
 $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);


Suggested change

$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);

Girgias · 2023-11-16T00:18:37Z

ext/pdo_mysql/tests/pdo_mysql_exec.phpt

@@ -178,7 +178,7 @@ MySQLPDOTest::skip();
 require __DIR__ . '/mysql_pdo_test.inc';
 $db = MySQLPDOTest::factory();
 $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);
-@$db->exec('DROP TABLE IF EXISTS test');
+@$db->exec('DROP TABLE IF EXISTS test_mysql_exec');


Suggested change

@$db->exec('DROP TABLE IF EXISTS test_mysql_exec');

$db->exec('DROP TABLE IF EXISTS test_mysql_exec');

Shouldn't be needed

Girgias · 2023-11-16T00:24:59Z

ext/pdo_mysql/tests/pdo_mysql_types_zerofill.phpt

-            // Zerofill implies unsigned but the test plays with signed = negative values as well!
+            // Zerofill implies unsigned but the test_mysql_types_zerofill plays with signed = negative values as well!


This "test" word does not need to be replaced

alexandre-daubois · 2023-11-16T13:45:48Z

Thanks, comments addressed 🙂

github-actions bot added the Extension: pdo_mysql label Aug 5, 2023

alexandre-daubois mentioned this pull request Aug 7, 2023

Improve ext/pdo_sqlite tests cleanup #11900

Merged

jorgsowa mentioned this pull request Oct 10, 2023

Parallelize pdo tests (dblib, firebird, oci, odbc, pgsql) #12395

Merged

Girgias reviewed Oct 10, 2023

View reviewed changes

kocsismate reviewed Oct 16, 2023

View reviewed changes

Girgias mentioned this pull request Oct 29, 2023

[Test] Optimized pdo_mysql tests #12555

Closed

KentarouTakeda mentioned this pull request Nov 4, 2023

Refactor ext/pgsql test #12608

Merged

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

https://github.com/php/php-src/pull/11879#discussion_r1352345791

ecdb457

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

334d883

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

a4ce140

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

7f997b9

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

ca5a482

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

improves php#11879 (comment)

42774ab

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

improves php#11879 (comment)

475f419

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

375d277

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment) php#11879 (comment)

610c9fb

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

0431358

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

4ba0aa7

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

8f77dcc

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

73a73bc

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

ecece3c

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

c45c8bd

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

ea82106

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

6aef7e8

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

7c1c2ad

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

improves php#11879 (comment)

0530611

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

improves php#11879 (comment)

23cd56b

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

8fedd85

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment) php#11879 (comment)

e94ff56

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

fb18fed

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

bf2d4ff

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

d4d4010

KentarouTakeda added a commit to KentarouTakeda/php-src that referenced this pull request Nov 11, 2023

closes php#11879 (comment)

e5efa38

KentarouTakeda mentioned this pull request Nov 11, 2023

Improve pdo mysql cleanup alexandre-daubois/php-src#2

Closed

alexandre-daubois force-pushed the improve-pdo_mysql-cleanup branch 2 times, most recently from 641b27f to ea4d805 Compare November 15, 2023 13:36

Girgias reviewed Nov 16, 2023

View reviewed changes

Improve ext/pdo_mysql tests cleanup

21f50f6

alexandre-daubois force-pushed the improve-pdo_mysql-cleanup branch from ea4d805 to 21f50f6 Compare November 16, 2023 12:03

Remove unnecessary DROP PROCEDURE

37d06ad

Girgias merged commit 6f95273 into php:master Nov 16, 2023

alexandre-daubois deleted the improve-pdo_mysql-cleanup branch January 28, 2024 08:19

iluuu1994 mentioned this pull request Mar 12, 2025

Fix flaky conneciton count in mysql test #18030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `ext/pdo_mysql` tests cleanup #11879

Improve `ext/pdo_mysql` tests cleanup #11879

alexandre-daubois commented Aug 5, 2023

javiereguiluz commented Aug 10, 2023

jorgsowa commented Oct 9, 2023 •

edited

Loading

Girgias left a comment

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

Girgias Oct 10, 2023

kocsismate left a comment

kocsismate Oct 16, 2023

kocsismate Oct 16, 2023

kocsismate Oct 16, 2023

kocsismate Oct 16, 2023

alexandre-daubois commented Oct 17, 2023

KentarouTakeda commented Nov 11, 2023

KentarouTakeda commented Nov 15, 2023

alexandre-daubois commented Nov 15, 2023

Girgias left a comment

Girgias Nov 16, 2023

alexandre-daubois Nov 16, 2023

Girgias Nov 16, 2023

alexandre-daubois Nov 16, 2023

Girgias Nov 16, 2023

Girgias Nov 16, 2023

Girgias Nov 16, 2023

alexandre-daubois commented Nov 16, 2023

	$db->exec("CREATE TABLE test_pcl_bug_5200 (bar INT NOT NULL, phase enum('please_select', 'I', 'II', 'IIa', 'IIb', 'III', 'IV'))");
	$db->exec("CREATE TABLE test_pecl_bug_5200 (bar INT NOT NULL, phase enum('please_select', 'I', 'II', 'IIa', 'IIb', 'III', 'IV'))");

		@@ -14,20 +14,20 @@ require_once(__DIR__ . DIRECTORY_SEPARATOR . 'mysql_pdo_test.inc');
		$pdo = MySQLPDOTest::factory();

		$tbl = "test";

	@$db->exec('DROP TABLE IF EXISTS test_mysql_exec');
	$db->exec('DROP TABLE IF EXISTS test_mysql_exec');

		// Zerofill implies unsigned but the test plays with signed = negative values as well!
		// Zerofill implies unsigned but the test_mysql_types_zerofill plays with signed = negative values as well!

Improve ext/pdo_mysql tests cleanup #11879

Improve ext/pdo_mysql tests cleanup #11879

Conversation

alexandre-daubois commented Aug 5, 2023

javiereguiluz commented Aug 10, 2023

jorgsowa commented Oct 9, 2023 • edited Loading

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandre-daubois commented Oct 17, 2023

KentarouTakeda commented Nov 11, 2023

KentarouTakeda commented Nov 15, 2023

alexandre-daubois commented Nov 15, 2023

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandre-daubois commented Nov 16, 2023

Improve `ext/pdo_mysql` tests cleanup #11879

Improve `ext/pdo_mysql` tests cleanup #11879

jorgsowa commented Oct 9, 2023 •

edited

Loading