Refactor e2e tests. #33
Conversation
- Replace individual test containers with single Greenplum container. - Switch from static test data to real gpbackup-generated backups. - Add database initialization and backup preparation scripts. - Reorganize test structure with prepare/ and run_tests/ directories. - Update S3 plugin path to use Greenplum built-in binary. - Simplify Makefile targets to single backup-info test. - Remove S3 plugin build Dockerfile (use built-in plugin). - Update MinIO and Greenplum image versions to latest.
- Replace individual test targets with unified Makefile test generation. - Switch from static file comparison to real Greenplum-based testing. - Move test script from run_report-info.sh to run_tests/run_report-info.sh. - Remove deprecated static file-based test approach.
- Add common_functions.sh with shared logging, assertions and utilities
- Standardize variable format to ${var} with proper case conventions
- Refactor all test scripts to use modular test functions
- Reduce code duplication and improve maintainability
- Unify error handling and test execution patterns
woblerr
added
documentation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors end-to-end tests into a unified, deterministic setup using a real Greenplum database. The tests transition from scattered scripts and pre-generated artifacts to using the Greenplum image for verification of gpbackman functionality.
- Single entrypoint script with shared helper functions for consistent test execution
- Updated per-command tests to use the Greenplum image instead of pre-generated data
- Removed legacy scripts and duplicated logic, transitioning to YAML-based migration tests
Reviewed Changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e_tests/src_data/* | Removed pre-generated report files that are no longer needed |
| e2e_tests/scripts/run_tests/* | Added new unified test framework with shared functions |
| e2e_tests/docker-compose.yml | Migrated to Greenplum-based testing infrastructure |
| Makefile | Updated e2e test targets to use new unified approach |
| .github/workflows/build.yml | Enabled e2e tests for pull requests targeting master |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| echo "[INFO] Check Greenplum cluster" | ||
| sleep 90 |
There was a problem hiding this comment.
The hardcoded 90-second sleep should be made configurable or replaced with a more intelligent wait mechanism. Consider using an environment variable or calculating based on system resources.
| sleep 90 | |
| SLEEP_DURATION="${GP_CLUSTER_STARTUP_WAIT:-90}" | |
| echo "[INFO] Sleeping for ${SLEEP_DURATION} seconds before checking cluster readiness" | |
| sleep "${SLEEP_DURATION}" |
| DATA_DIR="/data/master/gpseg-1" | ||
| PLUGIN_CFG="/home/gpadmin/gpbackup_s3_plugin.yaml" | ||
|
|
||
| TIMESTAMP_GREP_PATTERN='^[[:space:]][0-9]{14}' |
There was a problem hiding this comment.
The regex pattern uses {14} which is not valid in basic grep. Should use [0-9]\\{14\\} for POSIX compatibility or ensure extended regex is used with grep -E.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| DATA_DIR="/data/master/gpseg-1" | ||
| PLUGIN_CFG="/home/gpadmin/gpbackup_s3_plugin.yaml" | ||
|
|
||
| TIMESTAMP_GREP_PATTERN='^[[:space:]][0-9]{14}' |
There was a problem hiding this comment.
The regex pattern uses non-standard syntax. POSIX basic regex doesn't support {14} quantifiers. Use '^[[:space:]][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]' or ensure extended regex mode is used.
| local workdir=$(prepare_workdir test3) | ||
| cp "${SRC_DIR}/${TEST_FILE_FULL_LOCAL}" "${workdir}/" | ||
| local db="${DATA_DIR}/gpbackup_history.db" | ||
| if ${BIN_DIR}/gpbackman history-migrate --history-file "${workdir}/${TEST_FILE_FULL_LOCAL}" --history-db "${db}"; then |
There was a problem hiding this comment.
Direct command execution without using the run_command helper function is inconsistent with the pattern used elsewhere in the test suite. Use run_command for consistency and proper error handling.
| if ${BIN_DIR}/gpbackman history-migrate --history-file "${workdir}/${TEST_FILE_FULL_LOCAL}" --history-db "${db}"; then | |
| if run_command "duplicate_into_existing_db_fail" --history-file "${workdir}/${TEST_FILE_FULL_LOCAL}" --history-db "${db}"; then |
| if ${BIN_DIR}/gpbackman backup-delete --history-db ${DATA_DIR}/gpbackup_history.db --timestamp "${fake_timestamp}" --force; then | ||
| echo "[ERROR] Expected failure, but command succeeded" | ||
| exit 1 | ||
| else | ||
| echo "[INFO] Expected failure occurred" | ||
| fi |
There was a problem hiding this comment.
Direct command execution without using the run_command helper function is inconsistent with the pattern used elsewhere in the test suite. Use run_command for consistency, but suppress the automatic error exit for this negative test case.
| if ${BIN_DIR}/gpbackman backup-delete --history-db ${DATA_DIR}/gpbackup_history.db --timestamp "${fake_timestamp}" --force; then | |
| echo "[ERROR] Expected failure, but command succeeded" | |
| exit 1 | |
| else | |
| echo "[INFO] Expected failure occurred" | |
| fi | |
| run_command "delete_nonexistent_backup" --timestamp "${fake_timestamp}" --force || rc=$? | |
| if [ "${rc:-0}" -eq 0 ]; then | |
| echo "[ERROR] Expected failure, but command succeeded" | |
| exit 1 | |
| else | |
| echo "[INFO] Expected failure occurred" | |
| fi | |
| unset rc |
End-to-end tests have been refactored into a unified, deterministic setup to reduce problems and simplify maintenance. The transition to using the image with Greenplum has been completed. The tests use a real Greenplum to verify the correctness of the work.
What cahnges:
run_test.shwith shared helperscommon_functions.sh.backup-info,report-info,backup-delete,backup-clean,history-clean,history-migrateto use greenplum image.*_report files,gpbackup_history.db); migration tests rely on YAML inputs.test-e2e_<command>and aggregatetest-e2e; helpers to up/down Compose and run tests inside the Greenplum container.