Skip to content

tree_test - #6

Open
phbnf wants to merge 1 commit into
mainfrom
testonly
Open

tree_test#6
phbnf wants to merge 1 commit into
mainfrom
testonly

Conversation

@phbnf

@phbnf phbnf commented Jun 9, 2026

Copy link
Copy Markdown
Owner

No description provided.

@phbnf

phbnf commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new test function, TestTreeSubtreeInclusionProof, in testonly/tree_test.go to verify subtree inclusion proofs. The review feedback highlights two key areas for improvement: first, a readability enhancement to use the pre-defined end variable in the inner loop condition to avoid redundant type conversions; second, a bug fix in the golden test loop where the range bounds prevent the full slice of entries from being tested.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread testonly/tree_test.go Outdated
mt := newTree(entries)
end := uint64(len(entries))
for start := range end {
for index := start; index < uint64(len(entries)); index++ {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since end is already defined as uint64(len(entries)) on line 119, you can use end directly in the loop condition to improve readability and avoid redundant type conversions.

Suggested change
for index := start; index < uint64(len(entries)); index++ {
for index := start; index < end; index++ {

Comment thread testonly/tree_test.go Outdated

test("generated", genEntries(256))
entries := LeafInputs()
for end := range len(entries) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The loop for end := range len(entries) only iterates up to len(entries) - 1. This means the golden test case with the full slice of 8 elements is never tested. To include the full slice, you should iterate up to len(entries) inclusive.

Suggested change
for end := range len(entries) {
for end := range len(entries) + 1 {

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new test function, TestTreeSubtreeInclusionProof, to verify subtree inclusion proofs. The review feedback highlights three key areas for improvement: simplifying a loop condition by utilizing the pre-calculated end variable, refining the alignment check to cover more valid subtree ranges, and correcting a loop range to ensure the full slice of golden entries is tested.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread testonly/tree_test.go Outdated
mt := newTree(entries)
end := uint64(len(entries))
for start := range end {
for index := start; index < uint64(len(entries)); index++ {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The loop condition can be simplified by using the pre-calculated end variable instead of repeatedly calling uint64(len(entries)).

Suggested change
for index := start; index < uint64(len(entries)); index++ {
for index := start; index < end; index++ {

Comment thread testonly/tree_test.go Outdated
Comment on lines +122 to +124
if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 {
continue
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The alignment check can be made more precise to allow testing more valid subtree ranges (such as [2, 5)). The exact condition for the subtree [start, end) to have the same shape as a standalone tree of size end - start is that start must be aligned to the largest power of 2 less than or equal to end - start. This can be computed as uint64(1) << (bits.Len64(end-start) - 1).

Suggested change
if bc := uint64(1) << bits.Len64(end-start-1); start%bc != 0 {
continue
}
if bc := uint64(1) << (bits.Len64(end-start) - 1); start%bc != 0 {
continue
}

Comment thread testonly/tree_test.go Outdated
Comment on lines +140 to +142
for end := range len(entries) {
test(fmt.Sprintf("golden:%d", end), entries[:end])
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The loop using range len(entries) only iterates up to len(entries) - 1, which means the full slice of entries (i.e., entries[:len(entries)]) is never tested. To ensure complete coverage of all golden sizes, the loop should include len(entries).

Suggested change
for end := range len(entries) {
test(fmt.Sprintf("golden:%d", end), entries[:end])
}
for end := 0; end <= len(entries); end++ {
test(fmt.Sprintf("golden:%d", end), entries[:end])
}

@phbnf
phbnf force-pushed the testonly branch 2 times, most recently from c1c8012 to b2eeef8 Compare June 9, 2026 15:07
@phbnf

phbnf commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request adds a new test function, TestTreeSubtreeInclusionProof, to verify subtree inclusion proofs in testonly/tree_test.go. A review comment correctly identifies that the loop iterating over golden entries fails to test the full set of entries because range len(entries) stops at len(entries) - 1. The reviewer provides a code suggestion to fix this by iterating up to len(entries) + 1.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread testonly/tree_test.go Outdated
Comment on lines +141 to +143
for end := range len(entries) {
test(fmt.Sprintf("golden:%d", end), entries[:end])
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The loop for end := range len(entries) only iterates up to len(entries) - 1. This means the full set of golden entries (size 8) is never tested. To ensure the full tree size is covered by the tests, the loop should iterate up to len(entries) + 1.

Suggested change
for end := range len(entries) {
test(fmt.Sprintf("golden:%d", end), entries[:end])
}
for end := range len(entries) + 1 {
test(fmt.Sprintf("golden:%d", end), entries[:end])
}

@phbnf
phbnf force-pushed the testonly branch 4 times, most recently from 9a85ad2 to 2d24ab9 Compare June 16, 2026 10:16
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.

1 participant