Skip to content

Comments

add stackoverflow test for @immut/list.T#1788

Closed
illusory0x0 wants to merge 1 commit intomoonbitlang:mainfrom
illusory0x0:add-stackoverflow-test-for-immut-deque
Closed

add stackoverflow test for @immut/list.T#1788
illusory0x0 wants to merge 1 commit intomoonbitlang:mainfrom
illusory0x0:add-stackoverflow-test-for-immut-deque

Conversation

@illusory0x0
Copy link
Contributor

No description provided.

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

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

Test name contains a typo

Category
Correctness
Code Snippet
test "stackover flow" {
Recommendation
Rename the test to "stack overflow" to correctly represent the term
Reasoning
The term 'stackoverflow' is commonly written as two words 'stack overflow'. The current name could cause confusion or appear unprofessional.

Test assertions could benefit from a more descriptive comment

Category
Maintainability
Code Snippet
inspect!(lst.fold(init=0.0, fn(acc, x) { acc + x.to_double() }), content="549755289600")
Recommendation
Add a comment explaining the expected sum and why these three different approaches should yield the same result
Reasoning
The magic number 549755289600 and the purpose of testing three different folding approaches isn't immediately clear to other developers. A brief explanation would improve test readability and maintainability.

Multiple list traversals might be unnecessary for testing

Category
Performance
Code Snippet
let lst = from_iter(iter)
inspect!(lst.fold...)
inspect!(lst.rev_fold...)
inspect!(lst.rev().fold...)
Recommendation
Consider calculating the expected sum mathematically and store the list traversal result in a variable to avoid redundant computations
Reasoning
The current implementation traverses the large list three times. While this might be intentional for testing different methods, it could be optimized if the goal is just to verify the correctness of the operations.

@peter-jerry-ye
Copy link
Collaborator

Closing as cherry-picked to #1789

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