Skip to content

hongbo/coverage add #2437

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 13, 2025
Merged

hongbo/coverage add #2437

merged 3 commits into from
Jul 13, 2025

Conversation

bobzhang
Copy link
Contributor

  • docs: Clarify guidance on using inspect for tests and when to use assert_eq
  • Question: I have successfully increased the test coverage for the MoonBit standard library by creating comprehensive tests for multiple packages. Here's a summary of the improvements:

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link

Empty test files should be removed or contain actual test content

Category
Maintainability
Code Snippet
immut/array/tree_utils_test.mbt and immut/array/utils_test.mbt contain only empty lines
Recommendation
Either remove these empty files or add meaningful test content. Empty test files can confuse developers about test coverage.
Reasoning
Empty files in a test suite provide no value and can create confusion about what functionality is actually being tested.

Potential overflow check logic may be incorrect in checked_mul test

Category
Correctness
Code Snippet
// Test potential overflow
let large_val = 0xFFFFFFFFFFFFFFFFUL // max uint64
inspect(checked_mul(large_val, 2UL), content="None")
inspect(checked_mul(2UL, large_val), content="Some(18446744073709551614)")
Recommendation
Verify the expected behavior: if large_val * 2 overflows and should return None, then 2 * large_val should also overflow and return None due to multiplication commutativity.
Reasoning
The test expects different results for checked_mul(max_uint64, 2) vs checked_mul(2, max_uint64), which suggests inconsistent overflow detection or incorrect test expectations.

Test documentation could be more specific about when to use inspect vs assert_eq

Category
Maintainability
Code Snippet

  • When writing tests, you are encouraged to use inspect and run moon test --update to update the snapshots, only use assertions
    like assert_eq when you are in some loops where each snapshot may vary.
    Recommendation
    Provide more concrete examples: "Use inspect for deterministic outputs that can be snapshot-tested. Use assert_eq in loops or when testing dynamic/variable content where snapshot comparison would be unreliable."
    Reasoning
    The current guidance is somewhat vague about 'loops where each snapshot may vary' - clearer examples would help developers make better testing decisions.

@bobzhang bobzhang force-pushed the hongbo/coverage_add branch from b2ef504 to ed7ed62 Compare July 13, 2025 01:28
cursor[bot]

This comment was marked as outdated.

Sign in to view

@bobzhang
Question: I have successfully increased the test coverage for the Moo…
0726d8c 
…nBit standard library by creating comprehensive tests for multiple packages. Here's a summary of the improvements:

## Coverage Improvements:

**Overall Coverage**: Increased from **5,225/5,778 (90.4%)** to **5,238/5,778 (90.6%)**

### Files with Improved Coverage:

1. **immut/array/tree_utils.mbt**: 28/34 → 30/34 (coverage: 88.2%)
   - Added comprehensive whitebox tests for tree utility functions
   - Tested: `is_node()`, `is_leaf()`, `left_child()`, `right_child()`, `leaf_elements()`, `node_children()`, `local_size()`, `size()`

2. **strconv/number.mbt**: 82/97 → 84/97 (coverage: 86.6%)
   - Added whitebox tests for number parsing utilities
   - Tested: `parse_inf_nan()` with various cases including infinity, NaN, signed values, case-insensitive parsing
   - Tested: `checked_mul()` with edge cases including zero, one, and overflow scenarios

3. **hashset/hashset.mbt**: 173/186 → 180/186 (coverage: 96.8%)
   - Added comprehensive test cases for set operations
   - Tested: `is_disjoint()` with different set sizes, `is_subset()` with size comparisons, large set operations

4. **immut/array/utils.mbt**: Enhanced coverage through new whitebox tests
   - Tested utility functions: `immutable_set()`, `immutable_push()`, `shr_as_uint()`, `radix_indexing()`, `get_branch_index()`, `copy_sizes()`, `min()`

5. **char/char.mbt**: Enhanced with comprehensive character classification tests
   - Tested edge cases for ASCII classification, digit validation with different radixes, Unicode whitespace characters, numeric character testing, printable character validation, case conversion, and punctuation testing

## Test Files Created:

1. **immut/array/tree_utils_wbtest.mbt**: Whitebox tests for tree utilities (10 test cases)
2. **immut/array/utils_wbtest2.mbt**: Whitebox tests for array utilities (8 test cases)
3. **strconv/number_wbtest.mbt**: Whitebox tests for number parsing (7 test cases)
4. **hashset/hashset_coverage_test.mbt**: Coverage-focused hashset tests (5 test cases)
5. **char/char_coverage_test.mbt**: Comprehensive character tests (7 test cases)
@bobzhang bobzhang force-pushed the hongbo/coverage_add branch from ed7ed62 to 0726d8c Compare July 13, 2025 07:45
cursor[bot]

This comment was marked as outdated.

Sign in to view

@coveralls
Copy link
Collaborator

coveralls commented Jul 13, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 320

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 89.53%
Totals Coverage Status
Change from base Build 313: 0.2%
Covered Lines: 3489
Relevant Lines: 3897
💛 - Coveralls

bobzhang
bobzhang commented Jul 13, 2025
View reviewed changes
strconv/number_wbtest.mbt
}

///|
/// TODO: there is a bug in `checked_mul` that it does not handle
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @peter-jerry-ye the checked_mul is buggy, see the test here

@bobzhang bobzhang merged commit bffe4da into main Jul 13, 2025
11 checks passed
@bobzhang bobzhang deleted the hongbo/coverage_add branch July 13, 2025 07:59
@bobzhang bobzhang mentioned this pull request Jul 13, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cursor cursor[bot] cursor[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobzhang @coveralls