Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test hints #289

Merged
merged 2 commits into from
Sep 12, 2024
Merged

add test hints #289

merged 2 commits into from
Sep 12, 2024

Conversation

lijunchen
Copy link
Contributor

@lijunchen lijunchen commented Sep 12, 2024

moon test is different from other modes, this PR only adds hints for test

Copy link

peter-jerry-ye-code-review bot commented Sep 12, 2024

Issues Detected in the Provided git diff Output

  1. Typo in Variable Name:

    • In the file crates/moon/tests/test_cases/mod.rs, there is a repeated variable name failed in the println! statement. The variable failed is used twice, which could be a typo or an unintended repetition. It might be better to use different variable names for clarity.
    println!(
        "Total tests: {}, passed: {}, failed: {}.{}",
        if total > 0 {
            total.to_string().blue().to_string()
        } else {
            total.to_string()
        },
        if passed > 0 {
            passed.to_string().green().to_string()
        } else {
            passed.to_string()
        },
        if failed > 0 {
            failed.to_string().red().to_string()
        } else {
            failed.to_string()
        },
        backend_hint,
    );
  2. Unused Variable display_backend_hint:

    • In the file crates/moon/src/cli/test.rs, the variable display_backend_hint is declared but not used in the run_test_internal function. This variable is meant to provide a hint about the backend, but it seems to be unnecessary in the current context.
    fn run_test_internal(
        cli: &UniversalFlags,
        cmd: &TestSubcommand,
        source_dir: &Path,
        target_dir: &Path,
        display_backend_hint: Option<()>, // This variable is not used
    ) -> anyhow::Result<i32> {
        // Function body
    }
  3. Inconsistent Test Output:

    • In the file crates/moon/tests/test_cases/mod.rs, the test output format changes from a simple count to a more detailed format that includes the backend hint. This change might break existing tests or expectations in the test suite. It's important to ensure that all test cases are updated to match the new output format.
    // Old format
    Total tests: 1, passed: 0, failed: 1.
    
    // New format
    Total tests: 1, passed: 0, failed: 1. [wasm]

Suggestions

  1. Fix Typo:

    • Consider renaming the repeated failed variable to avoid confusion. For example, use failed_count or failures.
  2. Remove Unused Variable:

    • If the display_backend_hint variable is not used within the function, it should be removed to avoid confusion and potential bugs.
  3. Update Test Cases:

    • Ensure that all test cases and expectations are updated to match the new output format, including the backend hint. This will maintain consistency and prevent test failures due to mismatched output expectations.

}
let surface_targets = cmd.build_flags.target.clone().unwrap();
let targets = lower_surface_targets(&surface_targets);
if cmd.update && targets.len() > 1 {
return Err(anyhow::anyhow!("cannot update test on multiple targets"));
}
let display_backend_hint: Option<u16> = if targets.len() > 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we remove if we put backend into end ?

      Total tests: 0, passed: 0, failed: 0. [wasm]
      Total tests: 0, passed: 0, failed: 0. [wasm-gc]
      Total tests: 0, passed: 0, failed: 0. [js]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current situation, this is a good idea.

@lijunchen lijunchen merged commit 489b8f4 into main Sep 12, 2024
10 checks passed
@lijunchen lijunchen deleted the add-test-hints branch September 12, 2024 06:41
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