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

Auto add coverage when needed #415

Merged
merged 4 commits into from
Nov 6, 2024
Merged

Auto add coverage when needed #415

merged 4 commits into from
Nov 6, 2024

Conversation

lynzrand
Copy link
Collaborator

@lynzrand lynzrand commented Oct 23, 2024

Related Issues

Type of Pull Request

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

Does this PR change existing behavior?

  • Yes (please describe the changes below)
    • When compiling iff --enable-coverage is set and the module being built is moonbitlang/core, it automatically includes moonbitlang/core/coverage in the import list for each package. This is because the core library isn't included automatically in core builds, so coverage pacakges will need to be imported manually.
  • 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 Oct 23, 2024

From the provided git diff output, I've identified three notable changes and potential issues:

  1. Duplicate Coverage Import in Build Commands:

    • In the test cases (crates/moon/tests/test_cases/mod.rs), there are multiple instances where the coverage library is being imported twice (-i ./target/wasm-gc/debug/test/coverage/coverage.mi:coverage). This redundancy could lead to potential conflicts or unnecessary overhead.
    • Suggestion: Remove the duplicate import statements to streamline the build process and avoid potential issues.
  2. Uncommented Code in add_coverage_to_core_if_needed Function:

    • The function add_coverage_to_core_if_needed in crates/moonbuild/src/gen/gen_runtest.rs contains a commented-out block of code (// Add coverage library reference to each package). This might indicate that there's a consideration to add coverage library references to each package, but it's currently not implemented.
    • Suggestion: Review the commented-out code and decide whether it should be implemented or removed. If it's necessary, uncomment and ensure it works as intended. If not, remove it to avoid confusion.
  3. Potential Hardcoding of Module Names:

    • The constants MOONBITLANG_CORE and MOONBITLANG_COVERAGE are used directly in the add_coverage_to_core_if_needed function. This hardcoding might limit flexibility if the module names need to change in the future.
    • Suggestion: Consider using configuration settings or parameters to allow for dynamic module names. This would make the code more maintainable and adaptable to future changes.

These observations should help in refining the codebase for better performance and maintainability.

@lynzrand
Copy link
Collaborator Author

@lijunchen Do we need tests for this, and how if yes?

@lynzrand lynzrand force-pushed the rynco/auto-add-coverage branch from 5fca536 to 9661a31 Compare October 24, 2024 06:18
@lynzrand
Copy link
Collaborator Author

lynzrand commented Oct 24, 2024

A couple tests seem to be failing due to the new unused import warning, maybe we need a PR to update those before moving on?

cc @lijunchen

(Oops accidentally hit the close button)

@lynzrand lynzrand closed this Oct 24, 2024
@lynzrand lynzrand reopened this Oct 24, 2024
@lynzrand lynzrand requested a review from lijunchen October 24, 2024 06:27
@lynzrand lynzrand marked this pull request as ready for review October 24, 2024 06:27
@lijunchen
Copy link
Contributor

maybe we need a PR to update those before moving on?

ok

@lijunchen lijunchen requested review from Young-Flash and removed request for lijunchen October 24, 2024 07:13
@lynzrand lynzrand force-pushed the rynco/auto-add-coverage branch 3 times, most recently from 7f9466a to ad43545 Compare November 6, 2024 03:12
@lynzrand lynzrand force-pushed the rynco/auto-add-coverage branch from 7c4e3d1 to 73db67a Compare November 6, 2024 08:30
@lynzrand lynzrand enabled auto-merge November 6, 2024 08:30
@lynzrand lynzrand merged commit 79e45ae into main Nov 6, 2024
14 checks passed
@lynzrand lynzrand deleted the rynco/auto-add-coverage branch November 6, 2024 08:38
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