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

internal: add fs_sync package #1524

Closed
wants to merge 7 commits into from
Closed

internal: add fs_sync package #1524

wants to merge 7 commits into from

Conversation

Young-Flash
Copy link
Contributor

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Jan 21, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

⚠️ [Test leaves temporary files; potential cross-test contamination]
  • Category: Correctness
  • Code Snippet: blackbox_test.mbt test "write_and_read"
  • Recommendation: Add cleanup steps after assertions:
    // After inspections
    @fs_sync.delete_file!(path_1)
    @fs_sync.delete_file!(path_2)
  • Reasoning: The test creates files test_1.txt and test_2.txt but doesn't clean them up. This could cause false positives/negatives in subsequent test runs if files are not properly isolated.
⚠️ [Duplicate test execution steps in CI workflow]
  • Category: Maintainability
  • Code Snippet: .github/workflows/check.yml lines 266-276 vs 278-286
  • Recommendation: Consolidate test steps using matrix conditions:
    - name: Run moon tests
      run: |
        {% raw %}moon test --target all
        moon test --release --target all
        moon test --target native{% endraw %}
      if: matrix.os != 'windows-latest'
    - name: Run Windows tests
      run: same-as-above
      if: matrix.os == 'windows-latest'
  • Reasoning: Current implementation duplicates nearly identical test commands for Windows vs Unix-like systems, increasing maintenance burden.
⚠️ [Potential resource leak in native file handling]
  • Category: Correctness
  • Code Snippet: fs_sync_native.mbt read_file_to_bytes_internal (lines 59-69)
  • Recommendation: Ensure file closure using defer pattern:
    let file = fopen_ffi(...)
    guard is_null(file) == 0 else { ... }
    defer fclose_ffi(file) // Add this
    
    // Rest of logic...
  • Reasoning: If any guard condition fails after fopen_ffi, the file handle is never closed. This could lead to file descriptor leaks in long-running processes.

@Young-Flash Young-Flash force-pushed the sync_io branch 2 times, most recently from f184ce5 to b8d9b93 Compare January 21, 2025 06:48
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4884

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 16 of 20 (80.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 82.951%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sync_io/sync_io_wasm.mbt 3 4 75.0%
sync_io/sync_io.mbt 2 5 40.0%
Totals Coverage Status
Change from base Build 4879: 0.007%
Covered Lines: 5065
Relevant Lines: 6106

💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Jan 21, 2025

Pull Request Test Coverage Report for Build 4998

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 16 of 20 (80.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.827%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sync_io/sync_io_wasm.mbt 3 4 75.0%
sync_io/sync_io.mbt 2 5 40.0%
Totals Coverage Status
Change from base Build 4996: 0.0%
Covered Lines: 5233
Relevant Lines: 6169

💛 - Coveralls

sync_io/sync_io.mbti Outdated Show resolved Hide resolved
sync_io/sync_io.mbt Outdated Show resolved Hide resolved
@Young-Flash Young-Flash force-pushed the sync_io branch 5 times, most recently from d9ea73e to ac4aa27 Compare January 24, 2025 08:40
@Young-Flash Young-Flash changed the title internal: add sync_io package internal: add fs_sync package Jan 24, 2025
@Young-Flash Young-Flash force-pushed the sync_io branch 2 times, most recently from 00680b5 to 7500660 Compare January 24, 2025 09:25
@Young-Flash
Copy link
Contributor Author

failed at stable & nightly as expected.

@Young-Flash Young-Flash marked this pull request as ready for review January 24, 2025 09:38
///|
extern "js" fn read_file_ffi(path : String) -> Int =
#| function(path) {
#| fs = require('fs');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

var

@Young-Flash Young-Flash closed this Feb 5, 2025
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.

3 participants