Part 2 of X: Provide initial update to TRTC test suite #37

balanceiskey · 2025-03-25T19:23:06Z

This code begins to remodel taskruntoolcall_controller_test.go in the shape of taskrun_controller_test.go. There's more on its way to utils.go, so keep an eye on that file for updates. I've left the original tests commented out for the moment in taskruntoolcall_controller_test.go

Important

Update taskruntoolcall_controller_test.go to align with taskrun_controller_test.go and add utils_test.go for test utilities and resource management.

Test Suite Update:
- Remodel taskruntoolcall_controller_test.go to align with taskrun_controller_test.go.
- Comment out original tests in taskruntoolcall_controller_test.go for reference.
New Test Utilities:
- Add utils_test.go for setting up test resources and utilities.
- Define TestTool and TestTaskRunToolCall structs for test resource management.
- Implement Setup, SetupWithStatus, and Teardown methods for test lifecycle management.
Reconciliation Logic:
- Implement test reconciliation logic in taskruntoolcall_controller_test.go to handle task status updates and transitions.

^{This description was created by}^{for ac9e344. It will automatically update as commits are pushed.}

ellipsis-dev

❌ Changes requested. Reviewed everything up to ac9e344 in 3 minutes and 28 seconds

More details

Looked at 216 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 6 drafted comments based on config settings.

1. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go:24

Draft comment:
Avoid using the same name 'reconciler' for both the function and the variable. This shadowing may confuse readers.
Reason this comment was not posted:
Marked as duplicate.

2. kubechain/internal/controller/taskruntoolcall/utils_test.go:66

Draft comment:
Consider checking the error returned from k8sClient.Delete instead of discarding it. Ignoring deletion errors can mask unexpected issues.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is test setup code where we're just trying to ensure a clean slate before creating test resources. The Delete is explicitly marked as optional with "Delete if exists" comment. In test setup, it's common and acceptable to ignore "not found" errors from Delete operations. The subsequent Create and Get operations have proper error checking which would catch any serious issues.
The comment raises a valid point about error handling best practices in general. There could be unexpected issues that aren't "not found" errors.
While error checking is generally good practice, in test setup code it's common to ignore Delete errors since the main goal is just ensuring the resource doesn't exist before creating it. The subsequent operations have proper error checking.
The comment should be deleted as it suggests a change that isn't necessary in test setup code and wouldn't meaningfully improve the test's reliability.

3. kubechain/internal/controller/taskruntoolcall/utils_test.go:105

Draft comment:
Similarly, handle or document the deletion error in Setup for Tool (line 105) to avoid silent failures if deletion fails.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is test code where we're just trying to ensure a clean slate before creating test resources. The deletion is expected to fail if the resource doesn't exist. Adding error handling here would add noise to test code. The pattern of ignoring deletion errors in test setup is common and acceptable. The comment "// Delete if exists" shows this is intentional.
Maybe ignoring errors is a bad practice that could hide real issues. What if the deletion fails for an unexpected reason like permissions?
In test code, we have subsequent checks that would catch real issues - we verify the creation succeeds and can get the resource. The deletion is just cleanup and its failure doesn't impact test validity.
The comment should be deleted. Ignoring deletion errors is acceptable in test setup code, especially when it's clearly documented and followed by verification of the desired state.

4. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go:52

Draft comment:
Remove or archive the commented-out test code if no longer needed to improve code clarity.
Reason this comment was not posted:
Marked as duplicate.

5. kubechain/internal/controller/taskruntoolcall/utils_test.go:66

Draft comment:
Ignoring errors on Delete calls (e.g. '_ = k8sClient.Delete(...)') may mask unexpected issues. Consider handling non-NotFound errors explicitly.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
In test setup code, it's common and acceptable to ignore Delete errors since we're just trying to ensure a clean slate. The comment "Delete if exists" indicates this is intentional. The subsequent Create and Get operations have proper error checking. This is a test file where we care more about the test flow than production-grade error handling.
The comment raises a valid point about error handling in general, but may not understand this is test code with different requirements. There could be rare cases where Delete fails for reasons other than NotFound that might indicate test environment issues.
While thorough error handling is important in production code, test setup code often intentionally ignores certain errors for clarity and simplicity. The existing pattern is a common testing idiom.
The comment should be deleted as it suggests production-grade error handling in test code where the current approach is appropriate and commonly used.

6. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go:311

Draft comment:
There's a typographical error in the commented-out section at line 311: 'contrition' should likely be 'transition' (or a similar word that indicates switching states). Since this is in a comment, it's a minor, cosmetic issue. Please update to improve clarity.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_JKat2XDN1Nbb1o73

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-03-25T19:26:40Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go

+			taskRunToolCall := trtcForAddTool.Setup(ctx)
+
+			By("reconciling the taskruntoolcall")
+			reconciler, _ := reconciler()


Local variable reconciler shadows the function name reconciler(). Consider renaming the variable for clarity.

Suggested change

reconciler, _ := reconciler()

reconcilerInstance, _ := reconciler()

ellipsis-dev · 2025-03-25T19:26:41Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go

+	})
+})
+
+/*


Consider removing commented-out tests when no longer needed to keep the codebase clean.

dexhorthy · 2025-03-25T20:29:32Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go

+			Expect(err).NotTo(HaveOccurred())
+			Expect(updatedTRTC.Status.Phase).To(Equal(kubechainv1alpha1.TaskRunToolCallPhasePending))
+			Expect(updatedTRTC.Status.Status).To(Equal("Pending"))
+			Expect(updatedTRTC.Status.StatusDetail).To(Equal("Initializing"))


guessing we const these someday?

Yep, definitely coming soon.

Provide initial update to TRTC test suite

ac9e344

balanceiskey requested review from Timothy102, allisoneer and dexhorthy March 25, 2025 19:23

ellipsis-dev bot reviewed Mar 25, 2025

View reviewed changes

dexhorthy reviewed Mar 25, 2025

View reviewed changes

dexhorthy approved these changes Mar 25, 2025

View reviewed changes

balanceiskey merged commit cebf4c8 into humanlayer:main Mar 25, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Part 2 of X: Provide initial update to TRTC test suite #37

Part 2 of X: Provide initial update to TRTC test suite #37

Uh oh!

balanceiskey commented Mar 25, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot Mar 25, 2025

Uh oh!

dexhorthy Mar 25, 2025

Uh oh!

ellipsis-dev bot Mar 25, 2025

Uh oh!

dexhorthy Mar 25, 2025

Uh oh!

balanceiskey Mar 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	reconciler, _ := reconciler()
	reconcilerInstance, _ := reconciler()

+              	})
+              })
+              /*

Part 2 of X: Provide initial update to TRTC test suite #37

Part 2 of X: Provide initial update to TRTC test suite #37

Uh oh!

Conversation

balanceiskey commented Mar 25, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

dexhorthy Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

dexhorthy Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

balanceiskey Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

balanceiskey commented Mar 25, 2025 •

edited by ellipsis-dev bot

Loading