-
Notifications
You must be signed in to change notification settings - Fork 41
Fixed direct driver tests and test summary #260
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.
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
Conversation
bfb1ba2
to
510d0bc
Compare
Excellent work @p123-stack ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes issues with direct driver tests and updates the test summary along with several supporting refactorings.
- Updated tests to conditionally skip problematic tests in CI.
- Refactored response serialization, query type enum constants, and logging.
- Replaced the old ProfiledPlan with a new ProfiledQueryPlan and updated related data classes.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/Integration/ComplexQueryTest.php | Added CI check to skip tests affected by a known memory bug. |
testkit-backend/testkit.sh | Updated environment variables and test commands (with --teamcity flag). |
testkit-backend/src/Responses/SummaryResponse.php | Updated JSON serialization for summary responses and parameter formatting. |
testkit-backend/src/Handlers/TransactionRun.php | Added docblock for the request parameter. |
testkit-backend/src/Handlers/TransactionCommit.php | Changed transaction interface check to use UnmanagedTransactionInterface. |
testkit-backend/src/Backend.php | Removed duplicate logging and added a cutoff utility for long log messages. |
src/Formatter/SummarizedResultFormatter.php | Modified result summary formatting and introduced conversion and mapping improvements. |
src/Enum/QueryTypeEnum.php | Changed enum constant values to abbreviated forms. |
src/Databags/ServerInfo.php | Added a jsonSerialize method for consistent output. |
src/Databags/ResultSummary.php | Updated return types and documentation for timing values and plan types. |
src/Databags/ProfiledQueryPlan.php | Introduced a new data class replacing the old ProfiledPlan. |
src/Databags/ProfiledPlan.php | Removed the outdated ProfiledPlan class. |
src/Databags/Position.php | Renamed InputPosition to Position and adjusted property mutability and order. |
src/Databags/PlanArguments.php | Added a new class for plan argument encapsulation. |
src/Databags/Plan.php | Refactored the Plan class to use plain arrays and the new PlanArguments. |
src/Databags/Notification.php | Updated notification parsing logic and error handling in code splitting. |
.github/workflows/* | Updated workflow scripts to set CI environment variable and include --teamcity flag. |
Comments suppressed due to low confidence (3)
src/Databags/Position.php:22
- [nitpick] The Position class now uses mutable properties instead of being immutable as before. If immutability was intended, consider using readonly properties to enforce this behavior.
private int $column, private int $offset, private int $line,
src/Enum/QueryTypeEnum.php:37
- The constants in QueryTypeEnum have been abbreviated (e.g., 'r' instead of 'read_only'). Ensure these new values are aligned with all API consumers and that appropriate documentation updates are made if necessary.
private const READ_ONLY = 'r';
src/Databags/Notification.php:38
- The splitCode method expects the notification code to consist of at least 4 parts. Ensure that all notification codes conform to this format or consider adding fallback handling to avoid unexpected exceptions.
if (count($parts) < 4) {
(int) ($resultAvailableAfter * 1000), | ||
(int) ($resultConsumedAfter * 1000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Converting timing values to int by multiplying by 1000 may result in a loss of precision. Consider documenting the conversion behavior or using a rounding function to ensure consistency in the reported timings.
(int) ($resultAvailableAfter * 1000), | |
(int) ($resultConsumedAfter * 1000), | |
(int) round($resultAvailableAfter * 1000), // Rounded to ensure consistent millisecond precision | |
(int) round($resultConsumedAfter * 1000), // Rounded to ensure consistent millisecond precision |
Copilot uses AI. Check for mistakes.
No description provided.