Skip to content

[Tests] Optimized pdo_mysql tests #12751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 27, 2023
Merged

[Tests] Optimized pdo_mysql tests #12751

merged 3 commits into from
Nov 27, 2023

Conversation

SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi commented Nov 22, 2023 •
edited
Loading

take2 of #12555

Summary of changes

  • I organized the inc files into a directory
  • Optimized the config file and test classes
  • Eliminated all duplication that prevents tests from running in parallel
  • All $db is prepared using mysql_pdo_test.inc (excluding common.phpt)

Tests that have not been confirmed to work

I have confirmed that all tests except the ones listed below pass.

Tests that are probably always skipped:

  • pdo_mysql___construct_ini.phpt (test probably incomplete)
  • pdo_mysql_exec_load_data.phpt (local only)

only nightly (these run only with libmysql)

  • pdo_mysql___construct_options_libmysql.phpt
  • pdo_mysql_attr_max_buffer_size.phpt
  • pdo_mysql_bit.phpt
  • pdo_mysql_stmt_unbuffered_2050.phpt

@SakiTakamachi SakiTakamachi force-pushed the tests/optimize-pdo-mysql-tests-2 branch from 7581f9d to 6bd8ea5 Compare November 22, 2023 15:15
@SakiTakamachi
Copy link
Member Author

I don't have the concentration left, so I'll continue tomorrow.

@SakiTakamachi SakiTakamachi force-pushed the tests/optimize-pdo-mysql-tests-2 branch from 6bd8ea5 to 500b0fe Compare November 23, 2023 06:01
@SakiTakamachi SakiTakamachi force-pushed the tests/optimize-pdo-mysql-tests-2 branch from 4e2f843 to 5638afc Compare November 23, 2023 06:53
@SakiTakamachi SakiTakamachi force-pushed the tests/optimize-pdo-mysql-tests-2 branch from 5638afc to c7dd040 Compare November 23, 2023 08:54
@SakiTakamachi
Copy link
Member Author

done

@SakiTakamachi SakiTakamachi marked this pull request as ready for review November 23, 2023 10:30
@SakiTakamachi SakiTakamachi changed the title [WIP] [Tests] Optimized pdo_mysql tests [Tests] Optimized pdo_mysql tests Nov 23, 2023
Girgias
Girgias reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a partial review

ext/pdo_mysql/tests/bug_50323.phpt Outdated
require __DIR__ . '/../../../ext/pdo/tests/pdo_test.inc';
$db = PDOTest::test_factory(__DIR__ . '/common.phpt');
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
$db = MySQLPDOTest::factory();

@$db->exec('DROP DATABASE IF EXISTS `crazy;dbname`');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@$db->exec('DROP DATABASE IF EXISTS `crazy;dbname`');
$db->exec('DROP DATABASE IF EXISTS `crazy;dbname`');

Why is this error suppressed in the first place :(

ext/pdo_mysql/tests/pdo_mysql_pconnect.phpt Outdated
$tmp = $stmt->fetch(PDO::FETCH_ASSOC);
$con1 = $tmp['_con1'];

@$db2 = new PDO($dsn, $user, $pass, array(PDO::ATTR_PERSISTENT => true));
@$db2 = new PDO($dsn, $user, $pass, [PDO::ATTR_PERSISTENT => true]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@$db2 = new PDO($dsn, $user, $pass, [PDO::ATTR_PERSISTENT => true]);
$db2 = new PDO($dsn, $user, $pass, [PDO::ATTR_PERSISTENT => true]);

@SakiTakamachi
Copy link
Member Author

Thank you, Fixed everything except for error suppression. There was an oversight, so I fixed that as well.

When it comes to error suppression, I first need to understand what it want to do with the test, and I look at it one by one. This will take some time, so please be patient.

@SakiTakamachi SakiTakamachi force-pushed the tests/optimize-pdo-mysql-tests-2 branch from d9ec0d1 to db5b7da Compare November 24, 2023 11:44
@SakiTakamachi
Copy link
Member Author

Removed error suppression that could be removed.
There are still some, but I think it's okay to leave the rest as is because I'm expecting an error and am validating the error info and validating the return value.

Girgias
Girgias approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question but LGTM otherwise

ext/pdo_mysql/tests/bug_44707.phpt
$db = PDOTest::test_factory(__DIR__ . '/common.phpt');
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
$db = MySQLPDOTest::factory();
$db->setAttribute(PDO::ATTR_STRINGIFY_FETCHES, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this attribute being set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This attribute was set in PDOTest::test_factory, so the test will fail if it is not set.

Alternatively, there is a way to modify the expected value of the test.

@Girgias Girgias merged commit 4bb75d5 into php:master Nov 27, 2023
@SakiTakamachi
Copy link
Member Author

Thanks!

@SakiTakamachi SakiTakamachi deleted the tests/optimize-pdo-mysql-tests-2 branch November 28, 2023 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
Extension: pdo (core) Extension: pdo_mysql
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SakiTakamachi @Girgias