Skip to content

test: exercise resumable reviewer sessions #531

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.

Sign up for GitHub

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

Closed
rianjs wants to merge 1 commit into from
Closed

test: exercise resumable reviewer sessions #531

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/sessionprobe/probe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package sessionprobe

Check failure on line 1 in internal/sessionprobe/probe.go

View workflow job for this annotation

GitHub Actions / lint 

package-comments: should have a package comment (revive)

// Average returns the integer average of total across count items.
func Average(total, count int) int {
return total / count

@rianjs rianjs Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Average panics when count is zero because this return performs integer division without guarding the divisor. Any caller that passes 0 will crash instead of getting a defined result or error. Add a zero-count check before this line and return a safe value or surface the condition explicitly.

Reply inline to this comment.

@rianjs rianjs Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Average blindly divides by count, so the new helper panics when count == 0. For a reusable package-level function with a broad doc comment, that is a brittle contract: callers that summarize an empty slice or interrupted probe state will crash instead of getting defined handling. Add an explicit zero-count guard and make the contract testable, for example by returning an error/ok flag or by defining a sentinel result for zero. Whichever contract you choose, add a focused test that exercises the zero-count path so this does not regress silently.

Reply inline to this comment.

@rianjs rianjs Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Average panics when count == 0, but this new exported helper does not define or enforce any zero-count contract and the PR adds no focused test that would catch the edge case. That makes the behavior brittle for any future caller and leaves the change unproven. Define the zero case explicitly here (for example, return 0 or a typed error, depending on intended callers) and add a small table-driven test that covers both a normal average and the zero-count path.

Reply inline to this comment.

@rianjs rianjs Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This new exported helper panics when count == 0, but the change does not define that edge-case contract or add any focused test to lock intended behavior down. For a reusable package-level function, leaving zero-count as an implicit crash is brittle and easy for future callers to miss. Either guard count == 0 and return a defined result, or change the API to return an error/presence signal, then add a small table-driven test that covers both a normal average and the zero-count path.

Reply inline to this comment.

@rianjs rianjs Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This exported helper panics when count == 0, but the change does not define that edge-case contract or add any focused test that proves the intended behavior. For a reusable package-level function, leaving zero-count as an implicit crash is brittle for future callers. Either guard count == 0 and return a defined result, or change the API to return an error/presence signal, then add a small table-driven test covering both a normal average and the zero-count path.

Reply inline to this comment.

}
Loading