Fix multiple TAP tests [CI migration - mysql84-g1]#5469
Fix multiple TAP tests [CI migration - mysql84-g1]#5469rahim-kanji wants to merge 3 commits intov3.0from
Conversation
…stent schema. MySQL 8.4+ returns 1049 (Unknown database) for the same scenario. Accept both error codes for compatibility across versions.
Add TAP_REG_TEST_3549_AUTOCOMMIT_TRACKING___MYSQL_SERVER_HOSTGROUP export to infra .env files and source them in env-isolated.bash. Add INFRA_TYPE derivation from TAP_GROUP to ensure correct hostgroup (2900 for mysql84, 1300 for mysql57, etc.) is used for each infrastructure type.
📝 WalkthroughWalkthroughThis PR migrates test infrastructure configuration from a single hardcoded hostgroup value to a decentralized, infra-specific approach. It auto-derives the infrastructure type from test group names, conditionally sources per-infra environment files containing hostgroup settings, adds the hostgroup variable to multiple infra configurations, updates a ProxySQL user entry, and improves error message handling in a test. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several failing TAP tests, primarily in the context of a CI migration to a MySQL 8.4 environment. The changes focus on enhancing the test infrastructure's adaptability by dynamically configuring environment variables and hostgroups based on the specific test group. Additionally, it improves test robustness by accommodating varying error behaviors across different MySQL versions, ensuring tests pass consistently in the new CI setup. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses multiple TAP test failures for the mysql84-g1 CI migration. The changes primarily involve making the test environment setup more dynamic by sourcing infrastructure-specific configurations and deriving the infrastructure type from the TAP_GROUP. Additionally, a C++ test file is updated to accommodate different error codes across MySQL versions. The changes are logical and well-implemented. I have one suggestion to improve code clarity by using symbolic constants for error codes instead of magic numbers.
| // MySQL 5.7/8.0 returns 1044 (Access denied) when connecting to non-existent schema. | ||
| // MySQL 8.4+ returns 1049 (Unknown database) for the same scenario. | ||
| // Accept both error codes for compatibility across versions. | ||
| bool is_expected_error = (error_code == 1044 || error_code == 1049); |
There was a problem hiding this comment.
To improve readability and maintainability, it's better to use the symbolic constants for MySQL error codes instead of magic numbers. The header mysqld_error.h is already included in this file, so you can use ER_DBACCESS_DENIED_ERROR for 1044 and ER_BAD_DB_ERROR for 1049.
| bool is_expected_error = (error_code == 1044 || error_code == 1049); | |
| bool is_expected_error = (error_code == ER_DBACCESS_DENIED_ERROR || error_code == ER_BAD_DB_ERROR); |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/infra/control/env-isolated.bash (1)
105-108: Optional: add ShellCheck suppression for dynamicsource.This is intentional dynamic sourcing, but SC1090 will keep warning unless explicitly suppressed.
Proposed lint-only tweak
# Source infra-specific environment (exports WHG, RHG, and TAP test variables) if [ -n "${INFRA_TYPE}" ] && [ -f "${WORKSPACE}/test/infra/${INFRA_TYPE}/.env" ]; then + # shellcheck disable=SC1090 source "${WORKSPACE}/test/infra/${INFRA_TYPE}/.env" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/control/env-isolated.bash` around lines 105 - 108, Add a ShellCheck suppression for the intentional dynamic source to silence SC1090: update the dynamic sourcing block (the line that does source "${WORKSPACE}/test/infra/${INFRA_TYPE}/.env") to include a shellcheck directive (e.g., a preceding comment like "shellcheck disable=SC1090" or "shellcheck source=/dev/null") so the linter knows this dynamic source is intentional while leaving the runtime logic in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/infra/control/run-tests-isolated.bash`:
- Around line 39-54: INFRAS_TO_CHECK is populated from INFRA_TYPE before
INFRA_TYPE is derived from TAP_GROUP, which can leave INFRAS_TO_CHECK empty;
move the population of INFRAS_TO_CHECK to after the INFRA_TYPE derivation block
(the section referencing TAP_GROUP and export INFRA_TYPE) so the fallback path
gets the derived value, and add a case for TAP_GROUP patterns matching mysql57*
that maps to infra-mysql57 (referencing INFRA_TYPE, INFRAS_TO_CHECK, and
TAP_GROUP to locate the logic).
In `@test/infra/infra-mariadb10/.env`:
- Line 14: Remove the extra blank line flagged by dotenv-linter (ExtraBlankLine)
in test/infra/infra-mariadb10/.env so there are no consecutive empty lines;
simply delete the blank line at the reported location (line 14) to satisfy the
linter.
---
Nitpick comments:
In `@test/infra/control/env-isolated.bash`:
- Around line 105-108: Add a ShellCheck suppression for the intentional dynamic
source to silence SC1090: update the dynamic sourcing block (the line that does
source "${WORKSPACE}/test/infra/${INFRA_TYPE}/.env") to include a shellcheck
directive (e.g., a preceding comment like "shellcheck disable=SC1090" or
"shellcheck source=/dev/null") so the linter knows this dynamic source is
intentional while leaving the runtime logic in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b4ea004-f055-452b-84b6-ce3919b86ffb
📒 Files selected for processing (8)
test/infra/control/env-isolated.bashtest/infra/control/run-tests-isolated.bashtest/infra/infra-mariadb10/.envtest/infra/infra-mysql57-binlog/.envtest/infra/infra-mysql57/.envtest/infra/infra-mysql84/.envtest/infra/infra-mysql84/conf/proxysql/infra-config.sqltest/tap/tests/mysql_query_logging_memory-t.cpp
| # Derive INFRA_TYPE from TAP_GROUP if not set | ||
| # This is simple and deterministic - matches group naming convention | ||
| if [ -z "${INFRA_TYPE}" ]; then | ||
| case "${TAP_GROUP}" in | ||
| mysql84*) export INFRA_TYPE=infra-mysql84 ;; | ||
| mysql90*) export INFRA_TYPE=infra-mysql90 ;; | ||
| mysql91*) export INFRA_TYPE=infra-mysql91 ;; | ||
| mysql92*) export INFRA_TYPE=infra-mysql92 ;; | ||
| mysql93*) export INFRA_TYPE=infra-mysql93 ;; | ||
| mariadb*) export INFRA_TYPE=infra-mariadb10 ;; | ||
| legacy*) export INFRA_TYPE=infra-mysql57 ;; | ||
| esac | ||
| if [ -n "${INFRA_TYPE}" ]; then | ||
| echo ">>> Derived INFRA_TYPE from TAP_GROUP: ${INFRA_TYPE}" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Derivation order leaves INFRAS_TO_CHECK empty in the no-list path.
Line 35 populates INFRAS_TO_CHECK from INFRA_TYPE before INFRA_TYPE is derived at Line 41. In the fallback path, infra verification/default selection can be skipped unintentionally. Also, mysql57* is not mapped.
Proposed fix
-# If no list found, use INFRA_TYPE as single requirement
-if [ -z "${INFRAS_TO_CHECK}" ]; then
- INFRAS_TO_CHECK="${INFRA_TYPE}"
-fi
-
# Derive INFRA_TYPE from TAP_GROUP if not set
# This is simple and deterministic - matches group naming convention
if [ -z "${INFRA_TYPE}" ]; then
case "${TAP_GROUP}" in
mysql84*) export INFRA_TYPE=infra-mysql84 ;;
+ mysql57*) export INFRA_TYPE=infra-mysql57 ;;
mysql90*) export INFRA_TYPE=infra-mysql90 ;;
mysql91*) export INFRA_TYPE=infra-mysql91 ;;
mysql92*) export INFRA_TYPE=infra-mysql92 ;;
mysql93*) export INFRA_TYPE=infra-mysql93 ;;
mariadb*) export INFRA_TYPE=infra-mariadb10 ;;
legacy*) export INFRA_TYPE=infra-mysql57 ;;
esac
if [ -n "${INFRA_TYPE}" ]; then
echo ">>> Derived INFRA_TYPE from TAP_GROUP: ${INFRA_TYPE}"
fi
fi
+
+# If no list found, use INFRA_TYPE as single requirement
+if [ -z "${INFRAS_TO_CHECK}" ]; then
+ INFRAS_TO_CHECK="${INFRA_TYPE}"
+fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/control/run-tests-isolated.bash` around lines 39 - 54,
INFRAS_TO_CHECK is populated from INFRA_TYPE before INFRA_TYPE is derived from
TAP_GROUP, which can leave INFRAS_TO_CHECK empty; move the population of
INFRAS_TO_CHECK to after the INFRA_TYPE derivation block (the section
referencing TAP_GROUP and export INFRA_TYPE) so the fallback path gets the
derived value, and add a case for TAP_GROUP patterns matching mysql57* that maps
to infra-mysql57 (referencing INFRA_TYPE, INFRAS_TO_CHECK, and TAP_GROUP to
locate the logic).
| MARIADB2_PORT=${PREFIX}307 | ||
| MARIADB3_PORT=${PREFIX}308 | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove extra blank line to satisfy dotenv-linter.
Line 14 triggers the ExtraBlankLine warning.
Proposed fix
-
# Export hostgroup for TAP tests that need it
export TAP_REG_TEST_3549_AUTOCOMMIT_TRACKING___MYSQL_SERVER_HOSTGROUP=${WHG}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Export hostgroup for TAP tests that need it | |
| export TAP_REG_TEST_3549_AUTOCOMMIT_TRACKING___MYSQL_SERVER_HOSTGROUP=${WHG} |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 14-14: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-mariadb10/.env` at line 14, Remove the extra blank line
flagged by dotenv-linter (ExtraBlankLine) in test/infra/infra-mariadb10/.env so
there are no consecutive empty lines; simply delete the blank line at the
reported location (line 14) to satisfy the linter.
|
@rahim-kanji : maybe we should try to merge this into private/multi-group-runner (PR #5470) that already has a lot of enhancements |
2 participants
Summary by CodeRabbit
Chores
Tests