Skip to content

Task 2: Add 5 integration tests#180

Merged
genedna merged 4 commits intoweb3infra-foundation:mainfrom
xwwcyber:feat/r2cn-tests
Feb 1, 2026
Merged

Task 2: Add 5 integration tests#180
genedna merged 4 commits intoweb3infra-foundation:mainfrom
xwwcyber:feat/r2cn-tests

Conversation

@xwwcyber
Copy link
Contributor

@xwwcyber xwwcyber commented Feb 1, 2026

Description: I have added 5 new integration tests in tests/r2cn_submission_test.rs to cover various edge cases for the libra project.

Tests covered:

init idempotency (double init)

Adding a zero-byte empty file

Tracking files in deep directory structures

Handling file paths with spaces

Verifying status output accuracy

All tests passed successfully without warnings as shown below:

5_new_integration_tests

@genedna
Copy link
Member

genedna commented Feb 1, 2026

@xwwcyber , 这个集成测试的文件拆为 5 个文件,根据集成测试的实际名称来命名文件

@genedna genedna requested a review from Copilot February 1, 2026 10:20
@genedna
Copy link
Member

genedna commented Feb 1, 2026

@codex review

@genedna
Copy link
Member

genedna commented Feb 1, 2026

@claude

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2026

Claude finished @genedna's task in 1m 43s —— View job


Code Review Complete ✅

Todos

  • Read all 5 integration test files
  • Review Cargo.toml changes
  • Analyze test quality and implementation
  • Provide comprehensive review feedback

Summary

Good work splitting the tests into separate files as requested! The tests cover useful edge cases and the file structure is clean. However, there are several areas that need improvement before merging.

Overall Assessment

Strengths:

  • ✅ Tests are properly isolated in separate files with descriptive names
  • ✅ Good use of tempfile for test isolation
  • ✅ Documentation comments clearly explain test purpose
  • ✅ Tests cover important edge cases (empty files, deep directories, double init)
  • ✅ Proper dependency added to Cargo.toml (assert_cmd)

Issues Found:

🔴 Critical Issues

  1. Weak Assertions (tests/test_add_empty_file.rs:41, tests/test_add_sub_directory_file.rs:42)

    • Tests only verify command success, not actual behavior
    • Empty file and subdirectory file tests don't verify files were actually staged
    • Impact: Tests could pass even if the feature is broken
    • Fix: Add status verification like in test_status_after_add.rs
  2. Unclear Test Behavior (tests/test_double_init_warning.rs:28)

    • Test name implies checking for a warning, but no warning is verified
    • Comment says "behavior may vary", making expected behavior ambiguous
    • Impact: Test doesn't clearly document or verify expected behavior
    • Fix: Either verify warning output or rename test to reflect actual behavior being tested

🟡 Code Quality Issues

  1. Code Duplication (test_add_empty_file.rs, test_add_sub_directory_file.rs, test_status_after_add.rs)
    • Helper functions init_libra_repo() and setup_test_repo() duplicated across 3 files
    • Impact: Harder to maintain, violates DRY principle
    • Fix: Extract to shared test utilities module (e.g., tests/common/mod.rs)

🟢 Minor Suggestions

  1. Missing Test Case: No test for files with spaces in paths (mentioned in PR description but not present)

    • PR description claims to test "Handling file paths with spaces" but I don't see this test
    • Consider adding this test or updating the PR description
  2. Test Comparison with Existing Patterns:

    • Existing tests in tests/command/add_test.rs use #[tokio::test] and #[serial] attributes with internal API calls
    • New tests use CLI-based testing with assert_cmd, which is also valid but a different approach
    • Note: Both approaches are acceptable, but consider consistency across the test suite

Detailed Review

Code Quality: 6/10

  • Clean structure but weak assertions reduce effectiveness
  • Code duplication needs addressing

Testing: 5/10

  • Good coverage of edge cases
  • Critical gap: tests don't verify actual outcomes
  • Missing verification weakens test value significantly

Documentation: 8/10

  • Excellent doc comments on modules and functions
  • Clear explanations of test purpose

Security: ✅ N/A

  • No security concerns in test code

Performance: ✅ N/A

  • Test performance is adequate

Recommendations

Before merging:

  1. Must fix: Add proper assertions to verify files are actually staged in test_add_empty_file and test_add_sub_directory_file
  2. Must fix: Clarify test_double_init_warning - either verify warning output or rename to match actual behavior
  3. 🔵 Should fix: Extract duplicated helper functions to a shared module
  4. 🔵 Consider: Add the missing "files with spaces" test or update PR description

Example fix for weak assertions:

// After the add command, verify it was actually staged:
let output = Command::new(assert_cmd::cargo::cargo_bin!("libra"))
    .current_dir(dir.path())
    .arg("status")
    .output()
    .unwrap();
    
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(stdout.contains("empty.txt"), "File should be staged");

Verdict

Request Changes - The tests are well-structured but need stronger assertions to be truly effective. The critical issues with weak assertions must be addressed to ensure tests actually validate the intended behavior.


.current_dir(dir.path())
.arg("init")
.assert()
.success();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing assertion for expected behavior: The test name suggests this should verify a warning on double init, but there's no assertion checking for warning output. The comment on line 23 says "behavior may vary by implementation", which makes the test unclear.

Recommendation: Either:

  1. Assert that a specific warning message is displayed in stdout/stderr, or
  2. Assert that the .libra directory remains valid after double init, or
  3. Rename the test to clarify what behavior is actually being tested (e.g., test_double_init_idempotency)

Example assertion:

let output = Command::new(assert_cmd::cargo::cargo_bin!("libra"))
    .current_dir(dir.path())
    .arg("init")
    .output()
    .unwrap();
    
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(stderr.contains("already initialized") || stderr.is_empty());

use tempfile::{TempDir, tempdir};

/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication: The helper functions init_libra_repo and setup_test_repo are duplicated across 3 test files (test_add_empty_file.rs, test_add_sub_directory_file.rs, test_status_after_add.rs).

Recommendation: Extract these common helpers into a shared test utilities module to follow DRY principles:

  1. Create tests/common/mod.rs or tests/test_helpers.rs
  2. Move shared setup functions there
  3. Import in test files with use common::*;

This will make maintenance easier and ensure consistent test setup.

.arg("add")
.arg("empty.txt")
.assert()
.success();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weak assertion: The test only checks that the add command succeeds, but doesn't verify that the empty file was actually added to the staging area.

Recommendation: Add verification similar to test_status_after_add.rs to confirm the file appears in status output:

let output = Command::new(assert_cmd::cargo::cargo_bin!("libra"))
    .current_dir(dir.path())
    .arg("status")
    .output()
    .unwrap();
    
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(stdout.contains("empty.txt"), "Empty file should be staged");

.arg("add")
.arg("a/b/c/deep.txt")
.assert()
.success();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as test_add_empty_file: Only checks command success without verifying the file was actually staged. Consider adding status verification.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e54f5d57c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 23 to 28
// Second initialization should complete (behavior may vary by implementation)
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Expect re-init to succeed despite init returning AlreadyExists

The test asserts .success() on the second libra init, but the implementation explicitly treats re-initialization as an error and returns InitError::Io with AlreadyExists (see src/command/init.rs around the is_reinit check at lines ~439–446). With current behavior the CLI exits non‑zero, so this test will fail unless the command semantics are changed. If the intent is to warn and still succeed, the production code must be updated; otherwise the test should assert failure (or the specific warning on stderr).

Useful? React with 👍 / 👎.

.assert()
.success();

assert!(dir.path().join(".libra").exists());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good assertion: ✅ This test properly verifies the expected outcome (.libra directory exists). This is the level of verification the other tests should aim for.

.output()
.expect("Failed to execute status command");

let stdout = String::from_utf8_lossy(&output.stdout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good assertion with detailed error message: ✅ This is a good example of proper test verification - it checks the actual behavior and provides a clear failure message. However, the test doesn't verify the file shows up as "staged" vs just appearing in output.

Minor improvement suggestion: Consider also checking that the file is marked as "new" or "staged" in the status output to be more specific about what state it's in.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds 5 new integration test files intended to cover edge cases for the libra project, specifically: repository initialization in empty directories, double initialization idempotency, adding empty files, handling deep directory structures, and verifying status output after adding files.

Changes:

  • Added 5 new integration test files using assert_cmd for CLI testing
  • Added assert_cmd dependency (v2.1.2) to dev-dependencies
  • Tests use external command execution pattern rather than internal API calls

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
tests/test_init_in_empty_dir.rs Basic test verifying .libra directory creation on init
tests/test_double_init_warning.rs Tests that running init twice doesn't fail (idempotency)
tests/test_add_empty_file.rs Tests adding a zero-byte file to the repository
tests/test_add_sub_directory_file.rs Tests adding files in nested directory structures (a/b/c/deep.txt)
tests/test_status_after_add.rs Tests that status output includes added files
Cargo.toml Adds assert_cmd dev-dependency
Cargo.lock Adds assert_cmd and its transitive dependencies

Comment on lines 8 to 35
use tempfile::{TempDir, tempdir};

/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}

/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}

/// Test: Add an empty file to the repository
///
/// Ensures that Libra can handle adding empty files without errors
#[test]
fn test_add_empty_file() {
let dir = setup_test_repo();

let file_path = dir.path().join("empty.txt");
std::fs::File::create(&file_path).unwrap();

Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper functions init_libra_repo and setup_test_repo are duplicated identically across three test files (test_status_after_add.rs, test_add_sub_directory_file.rs, and test_add_empty_file.rs). This violates DRY principles and increases maintenance burden. Consider either: (1) following the existing pattern in tests/command/ which uses a shared test module with reusable utilities (see tests/command/mod.rs), or (2) creating a common test utilities module that these tests can import from.

Suggested change
use tempfile::{TempDir, tempdir};
/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}
/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}
/// Test: Add an empty file to the repository
///
/// Ensures that Libra can handle adding empty files without errors
#[test]
fn test_add_empty_file() {
let dir = setup_test_repo();
let file_path = dir.path().join("empty.txt");
std::fs::File::create(&file_path).unwrap();
use tempfile::tempdir;
/// Test: Add an empty file to the repository
///
/// Ensures that Libra can handle adding empty files without errors
#[test]
fn test_add_empty_file() {
// Initialize a temporary Libra repository
let dir = tempdir().unwrap();
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();
// Create an empty file in the repository
let file_path = dir.path().join("empty.txt");
std::fs::File::create(&file_path).unwrap();
// Add the empty file to the repository

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 53
//! Integration test: Status command after adding a file
//!
//! Verifies that the status command correctly reports added files

use std::{path::Path, process::Command};

use assert_cmd::prelude::*;
use tempfile::{TempDir, tempdir};

/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}

/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}

/// Test: Status command after adding a file
///
/// Verifies that the status command correctly reports added files
#[test]
fn test_status_after_add() {
let dir = setup_test_repo();

std::fs::write(dir.path().join("test.txt"), "content").unwrap();

Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("add")
.arg("test.txt")
.assert()
.success();

let output = Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("status")
.output()
.expect("Failed to execute status command");

let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stdout.contains("test.txt"),
"Status output should contain the added file"
);
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions testing "Handling file paths with spaces", but none of the added test files actually test this scenario. The test_status_after_add.rs test only covers a simple filename "test.txt" without any spaces. Either add a test that actually covers filenames with spaces, or update the PR description to accurately reflect what is being tested.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 53
//! Integration test: Status command after adding a file
//!
//! Verifies that the status command correctly reports added files

use std::{path::Path, process::Command};

use assert_cmd::prelude::*;
use tempfile::{TempDir, tempdir};

/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}

/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}

/// Test: Status command after adding a file
///
/// Verifies that the status command correctly reports added files
#[test]
fn test_status_after_add() {
let dir = setup_test_repo();

std::fs::write(dir.path().join("test.txt"), "content").unwrap();

Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("add")
.arg("test.txt")
.assert()
.success();

let output = Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("status")
.output()
.expect("Failed to execute status command");

let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stdout.contains("test.txt"),
"Status output should contain the added file"
);
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests use assert_cmd to invoke the libra CLI binary, which is a different approach from the existing integration tests in tests/command/. The existing tests (e.g., tests/command/add_test.rs, tests/command/status_test.rs) use internal API calls with tokio::test and serial attributes, importing from a shared test module. This inconsistency makes the test suite harder to maintain. Consider following the established pattern: use tokio::test, import test utilities from the command module (super::*), and call command execute functions directly rather than spawning external processes.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 29
//! Integration test: Attempt to initialize an already initialized repository
//!
//! Verifies that running init twice handles the situation appropriately

use std::process::Command;

use assert_cmd::prelude::*;
use tempfile::tempdir;

/// Test: Attempt to initialize an already initialized repository
///
/// Verifies that running init twice handles the situation appropriately
#[test]
fn test_double_init_warning() {
let dir = tempdir().unwrap();

Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();

// Second initialization should complete (behavior may vary by implementation)
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses assert_cmd to invoke the libra CLI binary, which differs from the established pattern in tests/command/. The existing tests use internal API calls with tokio::test and serial attributes. This inconsistency makes the test suite harder to maintain. Consider following the established pattern by calling the init command's execute function directly.

Copilot uses AI. Check for mistakes.
Comment on lines 23 to 28
// Second initialization should complete (behavior may vary by implementation)
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "behavior may vary by implementation" but the test only asserts success without verifying what actually happens. The test name includes "warning" but doesn't check for any warning output. Either verify that appropriate feedback is given (warning message or silent success), or update the test name and comment to reflect that it only tests idempotency (that double init doesn't fail).

Copilot uses AI. Check for mistakes.
Comment on lines 5 to 32
use std::{path::Path, process::Command};

use assert_cmd::prelude::*;
use tempfile::{TempDir, tempdir};

/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}

/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}

/// Test: Status command after adding a file
///
/// Verifies that the status command correctly reports added files
#[test]
fn test_status_after_add() {
let dir = setup_test_repo();

Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper functions init_libra_repo and setup_test_repo are duplicated identically across three test files (test_status_after_add.rs, test_add_sub_directory_file.rs, and test_add_empty_file.rs). This violates DRY principles and increases maintenance burden. Consider either: (1) following the existing pattern in tests/command/ which uses a shared test module with reusable utilities (see tests/command/mod.rs), or (2) creating a common test utilities module that these tests can import from.

Suggested change
use std::{path::Path, process::Command};
use assert_cmd::prelude::*;
use tempfile::{TempDir, tempdir};
/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}
/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}
/// Test: Status command after adding a file
///
/// Verifies that the status command correctly reports added files
#[test]
fn test_status_after_add() {
let dir = setup_test_repo();
use std::process::Command;
use assert_cmd::prelude::*;
use tempfile::tempdir;
/// Test: Status command after adding a file
///
/// Verifies that the status command correctly reports added files
#[test]
fn test_status_after_add() {
let dir = tempdir().unwrap();
// Initialize a new Libra repository in the temporary directory
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();

Copilot uses AI. Check for mistakes.
Comment on lines 48 to 52
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stdout.contains("test.txt"),
"Status output should contain the added file"
);
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion only checks that the string "test.txt" appears somewhere in the status output, which is a weak test. It doesn't verify the status prefix (e.g., "A " for added files in porcelain format) or the format of the output. The existing status tests in tests/command/status_test.rs show more rigorous verification patterns, checking specific status codes and output format. Consider strengthening this assertion to verify the actual status format.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 24
//! Integration test: Initialize Libra repository in an empty directory
//!
//! Verifies that the init command creates a .libra directory

use std::process::Command;

use assert_cmd::prelude::*;
use tempfile::tempdir;

/// Test: Initialize Libra repository in an empty directory
///
/// Verifies that the init command creates a .libra directory
#[test]
fn test_init_in_empty_dir() {
let dir = tempdir().unwrap();

Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("init")
.assert()
.success();

assert!(dir.path().join(".libra").exists());
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses assert_cmd to invoke the libra CLI binary, which differs from the established pattern in tests/command/. The existing tests (e.g., tests/command/init_test.rs) use internal API calls with tokio::test and serial attributes. This inconsistency makes the test suite harder to maintain. Consider following the established pattern by using test::setup_with_new_libra_in() from the shared test utilities as seen in tests/command/add_test.rs:12.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 43
//! Integration test: Add a file in a nested subdirectory
//!
//! Ensures that Libra correctly handles files in deep directory structures

use std::{path::Path, process::Command};

use assert_cmd::prelude::*;
use tempfile::{TempDir, tempdir};

/// Helper function to initialize a Libra repository in a temporary directory
fn init_libra_repo(dir: &Path) {
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir)
.arg("init")
.assert()
.success();
}

/// Helper function to create a test repository with initialization
fn setup_test_repo() -> TempDir {
let dir = tempdir().unwrap();
init_libra_repo(dir.path());
dir
}

/// Test: Add a file in a nested subdirectory
///
/// Ensures that Libra correctly handles files in deep directory structures
#[test]
fn test_add_sub_directory_file() {
let dir = setup_test_repo();

let sub_dir = dir.path().join("a/b/c");
std::fs::create_dir_all(&sub_dir).unwrap();
std::fs::write(sub_dir.join("deep.txt"), "hello deep").unwrap();

Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("add")
.arg("a/b/c/deep.txt")
.assert()
.success();
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test uses assert_cmd to invoke the libra CLI binary, which differs from the established pattern in tests/command/. The existing tests (e.g., tests/command/add_test.rs) use internal API calls with tokio::test and serial attributes. This inconsistency makes the test suite harder to maintain. Consider following the established pattern by calling add::execute() directly as seen in tests/command/add_test.rs:22.

Copilot uses AI. Check for mistakes.
Comment on lines 37 to 42
Command::new(assert_cmd::cargo::cargo_bin!("libra"))
.current_dir(dir.path())
.arg("add")
.arg("a/b/c/deep.txt")
.assert()
.success();
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test only asserts that the add command succeeds, but doesn't verify that the file was actually added to the index. Consider adding verification similar to the existing tests in tests/command/add_test.rs:35-37, which check that the file appears in changes_to_be_committed() after being added.

Copilot uses AI. Check for mistakes.
@genedna
Copy link
Member

genedna commented Feb 1, 2026

@xwwcyber , 请根据 Copilot 和 Claude 两个 AI Review 的分析,请判断是否需要修改,如无修改请在对应的 Review 信息下进行解释

@xwwcyber xwwcyber changed the title Complete R2CN Task 2: Add 5 integration tests Task 2: Add 5 integration tests Feb 1, 2026
@genedna genedna merged commit 32286b5 into web3infra-foundation:main Feb 1, 2026
10 checks passed
@genedna
Copy link
Member

genedna commented Feb 1, 2026

@xwwcyber , 请建一个 Issue 关联此任务,然后以 Issue 作为测试任务进行下一步注册,关于参与的任务请联系 genedna@qq.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants