Improve tests for various error scenarios#1642
Improve tests for various error scenarios#1642ggilder wants to merge 10 commits intogithub:masterfrom
Conversation
- Regex meta characters in index names should not break warning detection (required code fix) - Improve tests that only checked number of rows (need to validate data as well) - Test positive case allowing ignored duplicates on migration key - Test behavior with PanicOnWarnings disabled
There was a problem hiding this comment.
Pull request overview
This PR extends gh-ost’s applier warning-handling test suite (and a small part of the warning-matching logic) to better validate behavior around duplicate-key warnings, including regex metacharacters in key/table names, and to strengthen existing “no data loss” assertions.
Changes:
- Strengthen an existing test to validate not just row counts but the actual row contents after a duplicate-key warning scenario.
- Add new tests for: (1) allowing duplicates on the migration key during binlog replay, (2) behavior when
PanicOnWarningsis disabled, and (3) index names containing regex metacharacters. - Update warning matching in the applier to precompile a regex and escape table/key names via
regexp.QuoteMeta.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| go/logic/applier_test.go | Adds/extends tests for duplicate warnings (including intended regex-metachar coverage) and validates resulting ghost-table data. |
| go/logic/applier.go | Escapes and precompiles the migration-key warning regex in warning-checking paths to avoid regex metachar pitfalls and repeated compilation. |
Comments suppressed due to low confidence (1)
go/logic/applier.go:945
rows.Err()is checked before iteratingSHOW WARNINGS, but errors that occur during iteration are only surfaced after the loop. Add arows.Err()check after thefor rows.Next()loop (before storingMigrationLastInsertSQLWarnings) so iteration/driver errors don't get silently ignored inPanicOnWarningsmode.
sqlWarnings = append(sqlWarnings, fmt.Sprintf("%s: %s (%d)", level, message, code))
}
this.migrationContext.MigrationLastInsertSQLWarnings = sqlWarnings
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Print log excerpt on failure - Upload full log artifacts on failure
|
Working on fixing test flake issues here — will update when I have a passing build on my fork |
| exec_cmd() { | ||
| echo "$@" | ||
| command "$@" 1>$test_logfile 2>&1 | ||
| return $? | ||
| } | ||
|
|
| ARTIFACT_NAME=$(echo "${{ matrix.image }}" | tr '/:' '-') | ||
| echo "ARTIFACT_NAME=test-logs-${ARTIFACT_NAME}" >> $GITHUB_ENV | ||
| - name: Upload test logs on failure |
There was a problem hiding this comment.
This makes it much easier to debug failures
| with: | ||
| name: ${{ env.ARTIFACT_NAME }} | ||
| path: /tmp/gh-ost-test.* | ||
| retention-days: 7 |
There was a problem hiding this comment.
Open to shorter retention if desired
Removed the `wait.ForExposedPort()` override from test files. The tests
will now use the MySQL module's default wait strategy
(`wait.ForLog("port: 3306 MySQL Community Server")`), which properly
waits for MySQL to be ready to accept connections. Otherwise the port
may be exposed, but MySQL is still initializing and not ready to accept
connections.
Add support for test-specific execution so that we can guarantee that we're specifically testing the DML apply phase
| done | ||
| } | ||
|
|
||
| build_ghost_command() { |
| --execute ${extra_args[@]}" | ||
| } | ||
|
|
||
| validate_expected_failure() { |
| @@ -0,0 +1,44 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Ended up needing a custom script; mixing statements into create.sql that we wanted to occur during the binlog apply phase was way too flaky.
| testmysql.WithDatabase(testMysqlDatabase), | ||
| testmysql.WithUsername(testMysqlUser), | ||
| testmysql.WithPassword(testMysqlPass), | ||
| testcontainers.WithWaitStrategy(wait.ForExposedPort()), |
There was a problem hiding this comment.
I observed occasional flakes on the unit tests where they could not connect to MySQL. ForExposedPort checks for the port being exposed, but MySQL might not be done initializing so it can't accept connections. Removing this causes it to use the default wait strategy (wait.ForLog("port: 3306 MySQL Community Server")), which properly waits for MySQL to be ready to accept connections.
This is a follow up to #1633 — I realized there were some additional cases to test.
Regex meta characters in index names should not break warning detection (required code fix)
Improve tests that only checked number of rows (need to validate data as well)
Test positive case allowing ignored duplicates on migration key
Test behavior with PanicOnWarnings disabled
contributed code is using same conventions as original code
script/cibuildreturns with no formatting errors, build errors or unit test errors.