lmdb 5(b) Fix failing tests with LMDB#683
Open
subygan wants to merge 4 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughTests, helpers, and Windows CI setup were updated to use filesystem-backed merkle repositories and runner-temp test directories. ChangesFilesystem-backed merkle test harness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/liboxen/src/test.rs (1)
472-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original test error instead of collapsing it into
assert!(result).Any
OxenErrorfrom the callback becomes the same panic, which makes the newly migrated FS-backend tests much harder to diagnose. Clean up, then return the callback’sResultdirectly.Suggested fix
- let result = match test(repo).await { - Ok(_) => true, - Err(err) => { - eprintln!("Error running test. Err: {err}"); - false - } - }; - - maybe_cleanup_repo(&repo_dir)?; - assert!(result); - Ok(()) + let result = test(repo).await; + maybe_cleanup_repo(&repo_dir)?; + result🤖 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 `@crates/liboxen/src/test.rs` around lines 472 - 482, The test harness in test() is collapsing callback failures into a generic assert!(result) panic, which hides the original OxenError. Update the flow around match test(repo).await, maybe_cleanup_repo, and assert!(result) so cleanup still runs but the callback’s Result is returned directly instead of converted to a boolean/panic, preserving the original error context for failures.
🤖 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 `@crates/liboxen/src/test.rs`:
- Around line 435-441: The repository setup in test.rs only checks
config_filepath(path) after creating oxen_hidden_dir, so partially initialized
.oxen state can slip through. Update the helper that initializes the repo to
fail on any pre-existing hidden repo state by checking oxen_hidden_dir (or its
contents) before creating it, and keep the existing OxenError::basic_str
path-specific error in that initialization flow.
---
Nitpick comments:
In `@crates/liboxen/src/test.rs`:
- Around line 472-482: The test harness in test() is collapsing callback
failures into a generic assert!(result) panic, which hides the original
OxenError. Update the flow around match test(repo).await, maybe_cleanup_repo,
and assert!(result) so cleanup still runs but the callback’s Result is returned
directly instead of converted to a boolean/panic, preserving the original error
context for failures.
🪄 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: 709e7408-ce47-4afe-9882-08aaf1888341
📒 Files selected for processing (4)
crates/liboxen/src/api/client/tree.rscrates/liboxen/src/command/migrate/m20260626_migrate_merkle_nodes_to_lmdb.rscrates/liboxen/src/repositories/tree.rscrates/liboxen/src/test.rs
Comment on lines
+435
to
+441
| let hidden_dir = util::fs::oxen_hidden_dir(path); | ||
| util::fs::create_dir_all(&hidden_dir)?; | ||
| if util::fs::config_filepath(path).try_exists()? { | ||
| return Err(OxenError::basic_str(format!( | ||
| "Oxen repository already exists: {path:?}" | ||
| ))); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail on any pre-existing .oxen state, not just config.toml.
This helper creates .oxen first and only rejects when .oxen/config.toml already exists, so a partially initialized repo can be silently reused with stale tree/nodes contents. That breaks the clean-layout assumption the new FS-pinned tests rely on.
Suggested fix
pub fn init_fs_merkle_backend(path: &Path) -> Result<LocalRepository, OxenError> {
let hidden_dir = util::fs::oxen_hidden_dir(path);
- util::fs::create_dir_all(&hidden_dir)?;
- if util::fs::config_filepath(path).try_exists()? {
+ if hidden_dir.try_exists()? {
return Err(OxenError::basic_str(format!(
"Oxen repository already exists: {path:?}"
)));
}
+ util::fs::create_dir_all(&hidden_dir)?;
let config = RepositoryConfig {📝 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.
Suggested change
| let hidden_dir = util::fs::oxen_hidden_dir(path); | |
| util::fs::create_dir_all(&hidden_dir)?; | |
| if util::fs::config_filepath(path).try_exists()? { | |
| return Err(OxenError::basic_str(format!( | |
| "Oxen repository already exists: {path:?}" | |
| ))); | |
| } | |
| let hidden_dir = util::fs::oxen_hidden_dir(path); | |
| if hidden_dir.try_exists()? { | |
| return Err(OxenError::basic_str(format!( | |
| "Oxen repository already exists: {path:?}" | |
| ))); | |
| } | |
| util::fs::create_dir_all(&hidden_dir)?; |
🤖 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 `@crates/liboxen/src/test.rs` around lines 435 - 441, The repository setup in
test.rs only checks config_filepath(path) after creating oxen_hidden_dir, so
partially initialized .oxen state can slip through. Update the helper that
initializes the repo to fail on any pre-existing hidden repo state by checking
oxen_hidden_dir (or its contents) before creating it, and keep the existing
OxenError::basic_str path-specific error in that initialization flow.
subygan
force-pushed
the
ENG-1159/fix-test
branch
from
37d0d0d to
f2b83d3
Compare
subygan
changed the title
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
1 participant
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.
No description provided.