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

Trace correct test time #276

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Trace correct test time #276

merged 1 commit into from
Sep 12, 2024

Conversation

lgxbslgx
Copy link
Contributor

@lgxbslgx lgxbslgx commented Sep 9, 2024

Currently, when using the command moon test --trace to record the test time, the output in trace.json is not right. The duration is always 0 (shown below).

,{"pid":0, "name":"test", "ts":7753866, "tid": 0, "ph":"X", "dur":0}
,{"pid":0, "name":"test", "ts":7758979, "tid": 0, "ph":"X", "dur":0}

It is because the code in entry.rs just records the time to construct a Future instead of the time to run the test.

This PR relies on moonbitlang/n2#2 and can't be integrated before it. After merging these two PRs, the output should be right (as shown below).

,{"pid":0, "name":"test", "ts":9205546, "tid": 0, "ph":"X", "dur":401710}
,{"pid":0, "name":"test", "ts":9196616, "tid": 0, "ph":"X", "dur":414907}

Related Issues

  • Related issues: #____

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)
    • record the right test time
  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

Copy link

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

Observed Issues in the git diff Output

  1. Potential Synchronization Issue with Arc::clone:

    • The code uses Arc::clone(&printed) to clone an Arc reference. Ensure that the printed variable is properly synchronized across threads if it is being accessed concurrently. This is not a direct error but a potential concern for thread safety.
  2. Inconsistent Formatting in async move Block:

    • The original code had a trace::scope("test", || async { ... }) block, which was formatted to be more readable. However, the modified code introduces an async_scope function with a very long line, making it harder to read and understand at a glance. Consider formatting the async_scope call to align with typical Rust formatting conventions for better readability.
  3. Potential Misuse of await in async Block:

    • The original code had .await inside the trace::scope block, which was removed in the modified code. Ensure that the await keyword is correctly placed within the async block to avoid any potential issues with asynchronous execution. The await keyword is crucial for properly awaiting the result of an asynchronous operation, and its absence could lead to unexpected behavior.

Suggestions

  1. Review Thread Safety:

    • Ensure that the printed variable and any other shared mutable state are properly synchronized using appropriate locking mechanisms if needed.
  2. Improve Code Readability:

    • Refactor the async_scope call to improve readability, possibly by breaking it into multiple lines or using more descriptive variable names.
  3. Verify Correct Placement of await:

    • Double-check the placement of the await keyword within the async block to ensure that asynchronous operations are awaited correctly.

By addressing these points, the code can be made more robust, readable, and maintainable.

@lgxbslgx
Copy link
Contributor Author

What about this PR? I have another draft patch locally which is related to this patch and has git conflict with this patch. Kindly ping for review. Thanks a lot.

@@ -551,16 +551,16 @@ pub fn run_test(

let printed = Arc::clone(&printed);
handlers.push(async move {
let mut result = trace::scope("test", || async {
let mut result = trace::async_scope(
"test",
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could sepecify which test is running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several names which are passed to trace::scope need to be adjusted/polished. It is good to fix them in another patch.

@Young-Flash Young-Flash merged commit 0393383 into moonbitlang:main Sep 12, 2024
9 of 10 checks passed
@lgxbslgx lgxbslgx deleted the ASYNC branch September 12, 2024 08:35
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