Add FailurePolicyAction unit tests #967

carreter · 2025-07-31T22:20:32Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Adds unit tests for how FailurePolicyActions should modify the JobSet status when applied.

Which issue(s) this PR fixes:

Fixes #963

Special notes for your reviewer:

Conflicts with #958 due to the status API change. Whichever PR gets merged second will have to be updated.

Test rigging written by hand, specific test cases generated with Gemini 2.5 Pro and manually edited to fix. Quite surprised at how well it worked, honestly.

Does this PR introduce a user-facing change?

NONE

netlify · 2025-07-31T22:20:38Z

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	`d6da828`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-jobset/deploys/689a12bd07130900087f32cd

k8s-ci-robot · 2025-07-31T22:20:42Z

Hi @carreter. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

GiuseppeTT · 2025-07-31T23:31:13Z

/ok-to-test

GiuseppeTT · 2025-07-31T23:32:47Z

/assign @GiuseppeTT

GiuseppeTT · 2025-08-01T17:09:37Z

Conflicts with #958 due to the status API change. Whichever PR gets merged second will have to be updated.

I think it's best to merge #958 first. Besides, #958 looks ready for merging

carreter · 2025-08-01T18:40:12Z

Conflicts with #958 due to the status API change. Whichever PR gets merged second will have to be updated.

I think it's best to merge #958 first. Besides, #958 looks ready for merging

I'll make this stacked on top of #958 then!

carreter · 2025-08-04T19:22:45Z

Rebased on top of #958 and converted to draft until #958 merges.

GiuseppeTT · 2025-08-04T19:44:26Z

Rebased on top of #958 and converted to draft until #958 merges.

Let me know when it's ready for review. #958 just got merged

carreter · 2025-08-04T20:12:01Z

Sorry for the force-push, @GiuseppeTT this is ready for review.

GiuseppeTT · 2025-08-05T22:35:54Z

/lgtm

GiuseppeTT · 2025-08-05T22:36:14Z

/assign @andreyvelich

pkg/controllers/failure_policy_test.go

kannon92

One very minor typo:

#967 (comment)

andreyvelich

Thank you for this @carreter!

andreyvelich · 2025-08-08T21:01:54Z

pkg/controllers/failure_policy_test.go

+		matchingFailedJob      *batchv1.Job
+		failurePolicyAction    jobset.FailurePolicyAction
+		expectedJobSetStatus   jobset.JobSetStatus
+		shouldUpdateStatusOpts bool


Do we need this if that is always = true ?

Nice catch!

Fixed, thanks for the catch!

kannon92 · 2025-08-11T16:35:19Z

pkg/controllers/failure_policy_test.go

+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			if !updateStatusOpts.shouldUpdate {


Can you add a test case that doesn't require an update?

AFAIK, all failure policies result in an update. I need to double-check though.

kannon92

/lgtm
/approve

one minor nit: https://github.com/kubernetes-sigs/jobset/pull/967/files#r2267383874

k8s-ci-robot · 2025-08-11T16:35:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carreter, kannon92

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kannon92]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jul 31, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 31, 2025

k8s-ci-robot requested review from GiuseppeTT and danielvegamyhre July 31, 2025 22:20

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 31, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 31, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 31, 2025

k8s-ci-robot assigned GiuseppeTT Jul 31, 2025

kannon92 moved this to In Progress in Code to Review Aug 3, 2025

kannon92 added this to Code to Review Aug 3, 2025

kannon92 moved this from In Progress to Todo in Code to Review Aug 3, 2025

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2025

carreter marked this pull request as draft August 4, 2025 19:22

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2025

carreter added 2 commits August 4, 2025 20:02

Add FailurePolicyAction unit tests

5469070

Rebase on top of kubernetes-sigs#958

5a6323b

carreter force-pushed the failurepolicyaction-tests branch from 516e95a to 5a6323b Compare August 4, 2025 20:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 4, 2025

carreter marked this pull request as ready for review August 4, 2025 20:12

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2025

k8s-ci-robot assigned andreyvelich Aug 5, 2025

kannon92 moved this from Todo to In Progress in Code to Review Aug 8, 2025

kannon92 reviewed Aug 8, 2025

View reviewed changes

pkg/controllers/failure_policy_test.go Outdated Show resolved Hide resolved

kannon92 requested changes Aug 8, 2025

View reviewed changes

github-project-automation bot moved this from In Progress to PR has requested changes in Code to Review Aug 8, 2025

Typo

4ced4bc

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2025

carreter requested a review from kannon92 August 8, 2025 20:53

andreyvelich reviewed Aug 8, 2025

View reviewed changes

Remove unecessary test parameter

d6da828

carreter requested a review from andreyvelich August 11, 2025 15:57

kannon92 reviewed Aug 11, 2025

View reviewed changes

kannon92 approved these changes Aug 11, 2025

View reviewed changes

github-project-automation bot moved this from PR has requested changes to Done in Code to Review Aug 11, 2025

k8s-ci-robot assigned kannon92 Aug 11, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2025

k8s-ci-robot merged commit a4da72d into kubernetes-sigs:main Aug 11, 2025
12 checks passed

andreyvelich mentioned this pull request Aug 19, 2025

Release v0.9.0 #980

Closed

21 tasks

Add FailurePolicyAction unit tests #967

Add FailurePolicyAction unit tests #967

Uh oh!

Conversation

carreter commented Jul 31, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

netlify bot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Uh oh!

k8s-ci-robot commented Jul 31, 2025

Uh oh!

GiuseppeTT commented Jul 31, 2025

Uh oh!

GiuseppeTT commented Jul 31, 2025

Uh oh!

GiuseppeTT commented Aug 1, 2025

Uh oh!

carreter commented Aug 1, 2025

Uh oh!

carreter commented Aug 4, 2025

Uh oh!

GiuseppeTT commented Aug 4, 2025

Uh oh!

carreter commented Aug 4, 2025

Uh oh!

GiuseppeTT commented Aug 5, 2025

Uh oh!

GiuseppeTT commented Aug 5, 2025

Uh oh!

Uh oh!

kannon92 left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

kannon92 Aug 10, 2025

Choose a reason for hiding this comment

Uh oh!

carreter Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

kannon92 Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

carreter Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

kannon92 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented Jul 31, 2025 •

edited

Loading