test: add offline-mode regression tests for RAC fixtures#28
Conversation
Runs all 53 RAC fixtures through OLR's offline reader instead of batch. Validates archive discovery by directory scanning with no Oracle access and online-redo stripped from the checkpoint.
📝 WalkthroughWalkthroughChangesThis change adds a parametrized offline-mode test for RAC fixtures. It derives checkpoint and archive metadata, stages redo files, generates an OLR Docker configuration, runs OLR, and compares the resulting JSON with golden output. Offline RAC validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Pytest
participant OfflineConfig
participant OLR_Docker
participant FixtureFiles
Pytest->>OfflineConfig: discover checkpoint and archive metadata
OfflineConfig->>FixtureFiles: stage archives and write offline config
Pytest->>OLR_Docker: run OLR with mounted configuration
OLR_Docker-->>Pytest: produce output.json and return status
Pytest->>FixtureFiles: compare output.json with golden JSON
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/fixtures/test_fixtures_offline.py (1)
39-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilename-based archive-format detection is a fragile heuristic.
detect_archive_formatguesseslog-archive-formatfrom a single sample filename viarsplit("_", 2)plus a digit-stripping loop. This works for a simple<prefix><thread>_<seq>_<resetlogs>.extshape but breaks silently (producing a nonsensical format string) for other common Oracle archived-log naming conventions (e.g., OMF-style names with a trailing separator before the extension). Since this determines whether OLR can even discover the staged archives, a wrong guess would likely surface only as a confusing OLR failure rather than a clear test error.Since checkpoints/schema files may already encode redo log locations/format (per OLR's schema-file semantics), consider deriving the format from checkpoint metadata instead of reverse-engineering it from a filename sample, if such metadata is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixtures/test_fixtures_offline.py` around lines 39 - 54, Replace the filename-based heuristic in detect_archive_format with the archive format recorded in the available checkpoint or schema metadata, using the existing metadata representation and OLR schema-file semantics. Ensure the derived format is passed through unchanged for staged archive discovery, and add an explicit failure or validation path when the metadata does not provide a usable format instead of silently guessing from a sample filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fixtures/test_fixtures_offline.py`:
- Around line 23-36: Update the subprocess.run call in _run_olr to include a
finite timeout for the Docker-launched OLR process, using the test suite’s
established timeout configuration if available. Preserve the existing command
arguments and output-capture behavior while ensuring hangs raise the standard
subprocess timeout exception.
---
Nitpick comments:
In `@tests/fixtures/test_fixtures_offline.py`:
- Around line 39-54: Replace the filename-based heuristic in
detect_archive_format with the archive format recorded in the available
checkpoint or schema metadata, using the existing metadata representation and
OLR schema-file semantics. Ensure the derived format is passed through unchanged
for staged archive discovery, and add an explicit failure or validation path
when the metadata does not provide a usable format instead of silently guessing
from a sample filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 831e93f5-5acf-4dfc-819d-251a5322b2e0
📒 Files selected for processing (1)
tests/fixtures/test_fixtures_offline.py
- Add 120s timeout to docker run to prevent hanging tests - Read log-archive-format from checkpoint metadata first, fall back to filename inference only when checkpoint format doesn't match actual files
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/test_fixtures_offline.py (1)
49-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid relying strictly on
files[0]from directory globs.Evaluating only the first item makes the test fragile. If a hidden or unrelated file (such as
.DS_Store) is evaluated first, the format inference will fail or falsely reject the pattern, causing the test suite to crash.
tests/fixtures/test_fixtures_offline.py#L49-L62: Iterate overfilesand return the inferred format based on the first valid file that splits into at least 3 parts, instead of strictly assumingfiles[0]is a valid redo log.tests/fixtures/test_fixtures_offline.py#L77-L79: Useany(...)to check if the pattern matches any file in the directory, rather than only testingfiles[0].🛠️ Proposed fixes
For
_infer_format_from_filename(around lines 49-62):files = sorted(f for f in glob.glob(os.path.join(redo_dir, "*")) if os.path.isfile(f)) - if not files: - return None - fname = os.path.basename(files[0]) - stem, ext = os.path.splitext(fname) - parts = stem.rsplit("_", 2) - if len(parts) < 3: - return None - prefix_thread = parts[0] - i = len(prefix_thread) - while i > 0 and prefix_thread[i - 1].isdigit(): - i -= 1 - prefix = prefix_thread[:i] - return f"{prefix}%t_%s_%r{ext}" + for f in files: + fname = os.path.basename(f) + stem, ext = os.path.splitext(fname) + parts = stem.rsplit("_", 2) + if len(parts) >= 3: + prefix_thread = parts[0] + i = len(prefix_thread) + while i > 0 and prefix_thread[i - 1].isdigit(): + i -= 1 + prefix = prefix_thread[:i] + return f"{prefix}%t_%s_%r{ext}" + return NoneFor
_detect_archive_format(around lines 77-79):files = [os.path.basename(f) for f in glob.glob(os.path.join(redo_dir, "*")) if os.path.isfile(f)] - if files and re.fullmatch(pattern, files[0]): - return chkpt_fmt + if any(re.fullmatch(pattern, f) for f in files): + return chkpt_fmt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixtures/test_fixtures_offline.py` around lines 49 - 62, Update _infer_format_from_filename in tests/fixtures/test_fixtures_offline.py at lines 49-62 to iterate through files and return the format inferred from the first filename whose stem splits into at least three parts; return None only if no file is valid. Update _detect_archive_format at lines 77-79 to use any(...) so pattern matching succeeds when any directory file matches, rather than checking only files[0].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/fixtures/test_fixtures_offline.py`:
- Around line 49-62: Update _infer_format_from_filename in
tests/fixtures/test_fixtures_offline.py at lines 49-62 to iterate through files
and return the format inferred from the first filename whose stem splits into at
least three parts; return None only if no file is valid. Update
_detect_archive_format at lines 77-79 to use any(...) so pattern matching
succeeds when any directory file matches, rather than checking only files[0].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0893f0f5-56e4-4f8d-92d4-c058b7664e94
📒 Files selected for processing (1)
tests/fixtures/test_fixtures_offline.py
2 participants
Summary
tests/fixtures/test_fixtures_offline.py— runs all 53 RAC fixtures through OLR'sofflinereader modeonline-redostripped from the checkpointpath-mappingto redirect ASM paths to staged archive directoriesMotivation
The existing fixture regression tests only exercise
batchmode (explicit file list). This adds coverage for theofflinereader code path, which scans<db-recovery-file-dest>/<context>/archivelog/<date>/for archives matchinglog-archive-format.Test plan
Summary by CodeRabbit
output.jsonagainst golden expected results.