8000 chore: improve coverage for immut/hashset by Lampese · Pull Request #2310 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

chore: improve coverage for immut/hashset #2310

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

Merged
merged 1 commit into from
Jun 20, 2025
Merged

chore: improve coverage for immut/hashset #2310

merged 1 commit into from
Jun 20, 2025

Conversation

Lampese
Copy link
Collaborator
@Lampese Lampese commented Jun 20, 2025

No description provided.

Copy link
MyString implements Show twice with different methods

Category
Correctness
Code Snippet
impl Show for MyString with to_string(self) {...}
impl Show for MyString with output(self, logger) {...}
Recommendation
Combine the Show implementations into a single impl block or remove one if redundant
Reasoning
Having multiple implementations of the same trait can lead to ambiguity and potential runtime issues. The two Show implementations should be consolidated for clarity and correctness.

Hash implementation for MyString always returns constant value

Category
Maintainability
Code Snippet
impl Hash for MyString with hash_combine(_self, hasher) {
hasher.combine(1)
}
Recommendation
Use the actual string content for hashing: hasher.combine(self.inner().hash())
Reasoning
Using a constant hash value (1) for all strings defeats the purpose of hash-based collections and will lead to unnecessary collisions. While this might be intentional for testing, it should be documented if so.

Test descriptions could be more descriptive

Category
Maintainability
Code Snippet
test "HAMT::union with empty" {...}
test "HAMT::union with leaf" {...}
Recommendation
Add more descriptive test names that explain the expected behavior, e.g. "HAMT::union with empty set should return the non-empty set"
Reasoning
More descriptive test names serve as documentation and make it easier to understand test failures without diving into the implementation.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7418

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 90.097%

Totals Coverage Status
Change from base Build 7414: 0.2%
Covered Lines: 8525
Relevant Lines: 9462

💛 - Coveralls

@bobzhang bobzhang merged commit ce5d51e into main Jun 20, 2025
12 checks passed
@bobzhang bobzhang deleted the coverage-hamt branch June 20, 2025 23:27
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
0