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: add stack overflow test for unzip #1792

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

illusory0x0
Copy link
Contributor

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Mar 15, 2025

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

Based on the diff and pull request message, the purpose of this change appears to be adding a test case for stack overflow in the unzip operation of lists in MoonBit. The test creates a large list, zips it with itself, and then attempts to unzip it.

Here's my review:

💡 [Test case could be more descriptive and focused]
  • Category: Maintainability
  • Code Snippet:
test "stack overflow test/unzip" {
  let iter = Int::until(0, 1 << 13)
  let xs = from_iter(iter)
  let ys = xs
  let zs = xs.zip(ys)
  zs |> ignore
  zs.unwrap().unzip() |> ignore
}
  • Recommendation: Rename the test to be more specific and possibly split it into multiple focused tests
test "unzip should handle large lists without stack overflow" {
  let large_list = Int::until(0, 1 << 13) |> from_iter
  let zipped = large_list.zip(large_list)
  zipped.unwrap().unzip() |> ignore // Expect no stack overflow
}

test "zip should handle large lists" {
  let large_list = Int::until(0, 1 << 13) |> from_iter
  large_list.zip(large_list) |> ignore // Expect no stack overflow
}
  • Reasoning: The current test name is vague and the test is doing multiple things (testing both zip and unzip operations). By splitting it and using more descriptive names, the purpose of each test becomes clearer, making it easier to identify which specific functionality failed if there are issues.

This is the main issue I noticed. The test itself appears to be valid and correctly tests the stack safety of these operations, but could be improved for better maintainability and clarity. Otherwise, the change seems good and addresses an important edge case (large lists) that could cause stack overflow issues.

@bobzhang bobzhang enabled auto-merge (rebase) March 15, 2025 06:39
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 5707

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.654%

Totals Coverage Status
Change from base Build 5706: 0.0%
Covered Lines: 6029
Relevant Lines: 6507

💛 - Coveralls

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