test(sort): add more unit tests for pr sorting #2197
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @chmouel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I have focused on improving the test coverage for the PipelineRun sorting logic. This involved addressing a specific TODO comment by adding comprehensive test cases for sorting by completion time, including scenarios with missing or identical completion times. Additionally, I've corrected an existing test case for sorting by start time to accurately handle PipelineRuns that have not yet begun.
Highlights
- Enhanced PipelineRun Completion Time Sorting Tests: I've added a new test function,
TestPipelineRunSortByCompletionTime_missing, to cover edge cases for sorting PipelineRuns by completion time. This includes scenarios with empty lists, PipelineRuns having identical completion times, and those with anilcompletion time. - Corrected PipelineRun Start Time Sorting Test: I've fixed an existing test case in
TestPipelineRunSortByStartTimeto ensure it correctly handles PipelineRuns that have not yet started, by properly setting up the test data. - Code Cleanup: I've removed the
TODOcomment inpkg/sort/pipelinerun_test.goas the addressed test cases have now been implemented.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR enhances test coverage for PipelineRun sorting logic by adding additional test cases and fixing existing test issues. The changes address a TODO comment and improve validation of edge cases in the sorting functionality.
Key changes:
- Adds new test function to cover edge cases like empty lists, same completion times, and nil completion times
- Fixes incorrect test setup in existing StartTime sorting test
- Simplifies test structure by removing unnecessary nested test loops
Comments suppressed due to low confidence (2)
pkg/sort/pipelinerun_test.go:30
- [nitpick] The function name 'TestPipelineRunSortByCompletionTime_missing' is unclear. Consider renaming to 'TestPipelineRunSortByCompletionTime_EdgeCases' or 'TestPipelineRunSortByCompletionTime_NilCompletionTime' to better describe what scenarios are being tested.
func TestPipelineRunSortByCompletionTime_missing(t *testing.T) {
pkg/sort/pipelinerun_test.go:45
- The test doesn't verify the behavior for empty lists or PipelineRuns with the same completion time as mentioned in the PR description. Consider adding separate test cases for these scenarios.
wantName := []string{"no-completion-time", "premier", "troisieme"}
There was a problem hiding this comment.
Code Review
This pull request improves test coverage for PipelineRun sorting. It correctly fixes a failing test for sorting by start time and adds a new test case for sorting by completion time when a CompletionTime is nil. My main feedback is to refactor the tests for PipelineRunSortByCompletionTime back into a single, table-driven test. This improves maintainability and allows for easily adding more cases, such as the "empty list" case mentioned in the PR description. I've provided a suggestion to consolidate the tests.
|
trying gemini-code-assist on this repo... |
|
Hello @chmouel! I'm Gemini Code Assist, an AI assistant designed to help with code reviews. I can analyze your pull request for correctness, efficiency, maintainability, and security. To get a comprehensive code review of this pull request, please use the command |
chmouel
force-pushed
the
feature/add-tests-for-pipelinerun-sort
branch
from
8daa039 to
7f2c15c
Compare
zakisk
force-pushed
the
feature/add-tests-for-pipelinerun-sort
branch
from
7f2c15c to
dc58a82
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the test coverage for PipelineRun sorting logic by adding several new test cases, including for empty lists and PipelineRuns with nil timestamps. It also corrects a bug in the test setup for a PipelineRun that has not yet started. The changes are beneficial, and my feedback focuses on improving the maintainability and robustness of the new test code by reducing duplication and using more effective assertion patterns.
pkg/sort/pipelinerun_test.go
Outdated
| for i := range prunsMissing { | ||
| if prunsMissing[i].Name == "no-completion-time" { | ||
| prunsMissing[i].Status.CompletionTime = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code to set CompletionTime to nil is repeated for prunsWithOneMissing (lines 41-45) and prunsWithJMissing (lines 51-55). Consider creating the PipelineRun with nil completion time once and reusing it, or creating a helper function to reduce duplication and improve readability. This would make the test setup cleaner and easier to maintain.
pkg/sort/pipelinerun_test.go
Outdated
| for key, value := range got { | ||
| assert.Equal(t, tt.wantName[key], value.GetName()) | ||
| } |
There was a problem hiding this comment.
The current assertion loop has a couple of issues:
- It doesn't check if the length of the sorted slice matches the expected length. This could lead to panics or missed failures.
- The error message on failure is not very informative as it only shows the first mismatch.
A better approach is to collect the names from the sorted PipelineRuns into a slice and then use assert.Deep to compare it with the wantName slice. This provides a comprehensive diff on failure.
For example:
gotNames := make([]string, 0, len(got))
for _, pr := range got {
gotNames = append(gotNames, pr.GetName())
}
assert.Deep(t, tt.wantName, gotNames)Note: assert.Deep is available in gotest.tools/v3/assert and provides a rich diff.
chmouel
force-pushed
the
feature/add-tests-for-pipelinerun-sort
branch
from
dc58a82 to
827ece2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the test coverage for PipelineRun sorting functions by adding several new test cases for edge conditions like empty lists, single-item lists, and PipelineRuns with nil start/completion times. It also fixes a bug in an existing test. My review focuses on improving the clarity and maintainability of the new test code by suggesting more descriptive names for variables and test cases. I've also noted a missing test case that was mentioned in the PR description.
chmouel
force-pushed
the
feature/add-tests-for-pipelinerun-sort
branch
from
827ece2 to
a187ddd
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The code changes introduce new test cases for PipelineRun sorting, covering scenarios such as empty lists and items with missing completion/start times. A potential issue is identified in the test case for PipelineRuns with the same completion time, which relies on a stable sort but uses an unstable sorting function. The review suggests modifying the test to avoid dependence on the order of elements with the same completion time.
This commit addresses the TODO in `pkg/sort/pipelinerun_test.go` to add more test cases and improves the overall test coverage for the sorting logic. - Adds a new test function `TestPipelineRunSortByCompletionTime_missing` to cover more scenarios for sorting by completion time: - An empty list of PipelineRuns. - PipelineRuns with the same completion time. - A PipelineRun with a nil completion time. - Fixes a failing test case in `TestPipelineRunSortByStartTime` by correctly setting up the test data for a PipelineRun that has not started. - Removes the now-obsolete TODO comment. Fixes openshift-pipelines#2196 Signed-off-by: Chmouel Boudjnah <[email protected]>
chmouel
force-pushed
the
feature/add-tests-for-pipelinerun-sort
branch
from
a187ddd to
503beb7
Compare
This commit addresses the TODO in
pkg/sort/pipelinerun_test.goto add more test cases and improves the overall test coverage for the sorting logic.TestPipelineRunSortByCompletionTime_missingto cover more scenarios for sorting by completion time:TestPipelineRunSortByStartTimeby correctly setting up the test data for a PipelineRun that has not started.Fixes #2196
📝 Description of the Change
🔗 Linked GitHub Issue
Fixes #
👨🏻 Linked Jira
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)🧪 Testing Strategy
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.