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

fix update expect test #346

Merged
merged 1 commit into from
Sep 26, 2024
Merged

fix update expect test #346

merged 1 commit into from
Sep 26, 2024

Conversation

Young-Flash
Copy link
Collaborator

  • when updating expect test, if rerun test success, we should update the test resut directly and continue to process the next test result in the for loop
  • TestArgs should be passing to js_driver.js instead of hardcoding into it

Copy link

Observations and Suggestions

  1. Typo in inspect! Macro Call:

    • File: crates/moon/tests/test_cases/mod.rs
    • Line: + inspect!("d", content="d")
    • Issue: The inspect! macro call seems to have an unnecessary content="d" argument. According to the context provided, the inspect! macro likely does not take a content parameter.
    • Suggestion: Remove the content="d" argument to match the format of the previous inspect! call.
      - inspect!("d", content="d")
      + inspect!("d")
  2. Unused Code Removal:

    • File: crates/moonbuild/src/entry.rs
    • Lines:
      -                .replace(
      -                    "const testParams = []",
      -                    &format!("const testParams = {}", test_args.to_args()),
      -                )
      -                .replace(
      -                    "const packageName = \"\"",
      -                    &format!("const packageName = {:?}", test_args.package),
      -                );
    • Issue: The replacements for const testParams = [] and const packageName = "" are removed, but the corresponding to_args method and the package field in TestArgs are still present, suggesting they might be redundant.
    • Suggestion: Remove the to_args method and the package field in TestArgs if they are no longer needed.
      - fn to_args(&self) -> String {
      -     let file_and_index = &self.file_and_index;
      -     let mut test_params: Vec<[String; 2]> = vec![];
      -     for (file, index) in file_and_index {
      -         for i in index.clone() {
      -             test_params.push([file.clone(), i.to_string()]);
      -         }
      -     }
      -     format!("{:?}", test_params)
      - }
  3. Potential Logic Error in Rerun Handling:

    • File: crates/moonbuild/src/entry.rs
    • Lines:
      +                        // if rerun test success, update the previous test result and continue
      +                        Ok(_) => {
      +                            *item = rerun;
      +                            continue;
      +                        }
    • Issue: The comment suggests that if the rerun test is successful, the previous test result should be updated and the loop should continue. However, the code does not handle the case where the rerun test fails, which might be critical for error handling.
    • Suggestion: Ensure that the rerun failure case is handled appropriately to maintain the integrity of the test results.
      +                        Ok(_) => {
      +                            *item = rerun;
      +                            continue;
      +                        }
      +                        Err(e) => {
      +                            // Handle the error case appropriately
      +                        }

These observations should help improve the code quality and prevent potential issues in the future.

@Young-Flash Young-Flash merged commit b214601 into main Sep 26, 2024
10 checks passed
@Young-Flash Young-Flash deleted the fix_update_test branch September 26, 2024 07:09
@Young-Flash
Copy link
Collaborator Author

we should keep the ability to run test artifact without cli args for js backend, which used in ide debug
#351

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