Implement regression test for PXB 8.4.0-7 - #10
Open
saikumar-vs wants to merge 11 commits into
Open
saikumar-vs wants to merge 11 commits into
saikumar-vs wants to merge 11 commits into
Conversation
- Added `test_cloud_backup_md5_delete` to verify that `xbcloud delete` removes the associated .md5 sidecar file created by `xbcloud put --md5`. - Updated `test_helper.py` to include methods for listing and deleting S3 objects directly via REST API. - Enhanced `README.md` to document the new test and its requirements. - Adjusted `innodb_myrocks_backup_tests.py` to include the new test in the test suite. - Added checks to skip the test if the xtrabackup version is below 8.4.0-7, where the issue was fixed.
saikumar-vs
changed the title
test_helper.cpython-312.pyc was committed by mistake in df12676; add a .gitignore so pycache/compiled artifacts aren't tracked again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
curl exits 0 even when the S3 endpoint returns an HTTP error (e.g. 403 on a signature/credentials mismatch), so an auth or config problem was silently indistinguishable from "no matching objects" -- this is what caused test_cloud_backup_md5_delete to fail in CI with a confusing "found none among: []" instead of the real error. Both helpers now check the response's actual HTTP status and surface the S3 error body on failure. Also fixes reviewdog/ruff findings: unsorted import block in innodb_myrocks_backup_tests.py (I001) and List -> list annotation on s3_list_objects (UP006). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
curl's --aws-sigv4 auto-computes and sends x-amz-content-sha256 for a bodyless request on some curl builds but not others. Against MinIO this went unnoticed since it doesn't enforce the header, but real AWS S3 rejected the request outright: InvalidRequest: Missing required header for this request: x-amz-content-sha256 This is what test_cloud_backup_md5_delete hit in CI once the previous fix stopped swallowing the error silently. Pass the header explicitly (the well-known SHA-256 of an empty payload) so the request no longer depends on the CI host's curl version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The test hardcoded databases = ["test"], so when ROCKSDB=enabled (as it is in the pxb-new-ps-innodb-rocksdb CI job), RocksDB data was still backed up and restored by xtrabackup (a full physical backup captures everything regardless of storage engine) but never actually verified: no row-count/checksum comparison and no CHECK TABLE against test_rocksdb. Fold RocksDB in the same way the rest of this suite already does (e.g. test_copy_back_largest_file_first): databases = ["test", "test_rocksdb"] if rocksdb_enabled else ["test"], and loop check_tables over that same list instead of hardcoding "test". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ruff/reviewdog flagged the naive datetime.now() call added for test_cloud_backup_md5_delete's log_date. It's only ever used to build unique backup names and log-file suffixes, so switching to UTC is a pure naming change with no effect on test behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PEP 484 prohibits implicit Optional; storage_engine: str = None should be storage_engine: str | None = None. Type-annotation-only change, no behavior difference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the earlier md5-delete test: test_decompress_largest_file_first and test_copy_back_largest_file_first also called datetime.now() without tz for their log_date, flagged by reviewdog on this PR's diff. Both uses are purely for backup/log-file-name suffixes, so tz=timezone.utc is a pure naming change with no behavior difference. Verified both tests still pass locally (PXB 8.4.0-7). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
saikumar-vs
marked this pull request as ready for review
September 21, 2026 06:30
The RUF013 fix for _sysbench_prepare's storage_engine parameter used PEP 604 union syntax (str | None), which is only valid at runtime on Python 3.10+ since this file has no `from __future__ import annotations`. The oracle-9 CI host runs Python 3.9, where importing this module raised: TypeError: unsupported operand type(s) for |: 'type' and 'NoneType' at collection time, breaking every test in the file, not just this one. typing.Optional[str] is what every other annotation in this codebase already uses and works on 3.9+; ruff's suggested `str | None` fix didn't account for the project's actual minimum Python version. Confirmed: reproduced the failure importing this module under python:3.9-slim, confirmed this fix resolves it, and re-ran test_cloud_backup_md5_delete, test_decompress_largest_file_first, and test_copy_back_largest_file_first locally (Python 3.12) -- all still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ruff had no configured target-version, so it defaulted to assuming the newest Python and suggested syntax like `X | None` (UP045) that raises TypeError at import time on Python 3.9 -- e.g. Oracle/RHEL 9's system python3, which is what broke test_cloud_backup_md5_delete on the oracle-9 CI host. This pins ruff's baseline to the actual oldest confirmed interpreter in the CI fleet so its suggestions stay valid for every host that runs these tests, instead of silently drifting ahead of them one flagged line at a time. Verified: `ruff check --show-settings` confirms the config is picked up automatically (matches how the CI workflow invokes ruff, no explicit --config flag anywhere in lint.yml); UP045 no longer fires on the Optional[str] annotation that broke oracle-9; UP006 (list[str], safe on 3.9+ via PEP 585) is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tplavcic
requested changes
Sep 22, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test_cloud_backup_md5_deleteto verify thatxbcloud deleteremoves the associated .md5 sidecar file created byxbcloud put --md5.test_helper.pyto include methods for listing and deleting S3 objects directly via REST API.README.mdto document the new test and its requirements.innodb_myrocks_backup_tests.pyto include the new test in the test suite.https://pxb.cd.percona.com/view/QA/job/pxb-new-ps-innodb-rocksdb/82/testReport/(root)/
full run =https://pxb.cd.percona.com/view/QA/job/pxb-new-ps-innodb-rocksdb/88/testReport/(root)/