moonbitlang / moon

The build system and package manager for MoonBit
https://moonbitlang.github.io/moon/
GNU Affero General Public License v3.0
140 stars 11 forks source link

Trace correct test time #276

Closed lgxbslgx closed 5 days ago

lgxbslgx commented 1 week ago

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 https://github.com/moonbitlang/n2/pull/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

Type of Pull Request

Does this PR change existing behavior?

Does this PR introduce new dependencies?

Checklist:

peter-jerry-ye-code-review[bot] commented 1 week ago

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 commented 5 days ago

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.