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

test: improve coverage for immut/hashmap/HAMT.mbt #1562

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Jan 24, 2025

Disclaimer: This PR was generated by an LLM agent as part of an experiment.

Summary

coverage of `immut/hashmap/HAMT.mbt`: 72.7% -> 80.3%

**Disclaimer:** This PR was generated by an LLM agent as part of an experiment.

## Summary

```
coverage of `immut/hashmap/HAMT.mbt`: 72.7% -> 80.3%
```
Copy link

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

⚠️ [Tests may not correctly simulate hash collisions]
  • Category: Correctness
  • Code Snippet:
    test "remove non-existent key with hash collision" {
      let map = @hashmap.of([("a", 1)])
      let new_map = map.remove("b")
      inspect!(new_map.size(), content="1")
    }
    
    test "remove all keys from collision bucket" {
      let map = @hashmap.from_array([("a", 1), ("b", 2)])
      let map1 = map.remove("a")
      let map2 = map1.remove("b")
      inspect!(map2.size(), content="0")
    }
  • Recommendation: Use keys with known hash collisions or modify test setup to force collisions
  • Reasoning: These tests appear to validate collision handling but use different string keys ("a"/"b") without verifying they actually collide. If the default string hashing doesn't produce collisions for these values, the tests aren't validating the collision scenarios they purport to test, potentially leaving collision-related bugs undetected.
💡 [Consider adding collision verification precondition]
  • Category: Correctness
  • Code Snippet:
    test "remove non-existent key with hash collision" {
      let map = @hashmap.of([("a", 1)])
      // Add precondition check here
  • Recommendation: Add assertion that "a" and "b" actually have the same hash
  • Reasoning: Ensures the test is validating the intended collision scenario. For example: assert(hash("a") == hash("b"), "Test requires hash collision") before main test logic.
⚠️ [Potential type inconsistency in empty map test]
  • Category: Correctness
  • Code Snippet:
    test "each on empty map" {
      let empty = @hashmap.new()
      let mut sum = 0
      empty.each(fn(k : Int, v : Int) { sum = sum + k + v })
  • Recommendation: Verify map's key-value types match callback parameters
  • Reasoning: The @hashmap.new() appears to create a generic map, but the callback expects Int keys/values. If the empty map isn't explicitly typed as HashMap[Int, Int], this could cause type mismatches when used with non-integer entries later.

@rami3l rami3l enabled auto-merge January 24, 2025 02:50
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 5059

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 85.324%

Totals Coverage Status
Change from base Build 5056: 0.08%
Covered Lines: 5244
Relevant Lines: 6146

💛 - Coveralls

@rami3l rami3l added this pull request to the merge queue Jan 24, 2025
Merged via the queue into moonbitlang:main with commit b2878ed Jan 24, 2025
14 checks passed
@rami3l rami3l deleted the regolith/cov-immut-hashmap-HAMT-mbt branch January 24, 2025 03:19
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