Extend unit-test coverage for jobframework #979

stuton · 2023-07-12T15:35:01Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #922

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

* New tests validate that job is suspended if it's parent workload is not found, or if it's admitted.

k8s-ci-robot · 2023-07-12T15:35:11Z

Hi @stuton. 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.

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/test-infra repository.

netlify · 2023-07-12T15:35:25Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`164145d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64c2344d253b230008b93c3c

stuton · 2023-07-12T15:37:34Z

/assign @trasc

alculquicondor · 2023-07-12T15:38:24Z

/ok-to-test

trasc

Can you add some scenarios for suspending/unsuspending the job (verifying how the node selectors and job counts (parraleism) are managed).

pkg/controller/jobs/job/job_controller_test.go

alculquicondor · 2023-07-21T18:05:57Z

Makefile

@@ -149,7 +149,7 @@ vet: ## Run go vet against code.

 .PHONY: test
 test: generate gotestsum ## Run tests.
-	$(GOTESTSUM) --junitfile $(ARTIFACTS)/junit.xml -- $(GO_TEST_FLAGS) $(shell $(GO_CMD) list ./... | grep -v '/test/') -coverprofile $(ARTIFACTS)/cover.out
+	$(GOTESTSUM) --junitfile $(ARTIFACTS)/junit.xml -- $(GO_TEST_FLAGS) $(shell $(GO_CMD) list ./... | grep -v '/test/') -coverpkg=./... -coverprofile $(ARTIFACTS)/cover.out


what is this change for?

since we are using reconciler in integration tests we need to enable coverage of this external package. This option helps calculate coverage for reconciler package

pkg/controller/jobframework/reconciler.go

pkg/controller/jobs/job/job_controller_test.go

pkg/controller/jobs/jobset/jobset_controller_test.go

pkg/controller/jobs/mpijob/mpijob_controller_test.go

alculquicondor · 2023-07-21T18:26:19Z

test/integration/controller/job/job_controller_test.go

@@ -434,58 +434,6 @@ var _ = ginkgo.Describe("Job controller", ginkgo.Ordered, ginkgo.ContinueOnFailu
 	})
 })

-var _ = ginkgo.Describe("Job controller for workloads with no queue set", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() {


Awesome, reducing integration tests without losing coverage is a good outcome

mimowo

/lgtm
Just some non-blocking nits, like adding test cases, but they can be added in follow ups

pkg/controller/jobs/job/job_controller_test.go

pkg/util/testing/wrappers.go

Added two unit tests for reconciler.go

mimowo

/lgtm
+1, The unit tests run much faster than integration tests and allow to easily use debugger. While there is value in integration tests, we can add less of them for the new integrations.
/assign @alculquicondor

mimowo · 2023-07-27T09:25:50Z

pkg/controller/jobframework/reconciler.go

 		}
-		return nil, fmt.Errorf("%w: deleted %d workloads", ErrExtraWorkloads, len(workloads.Items))
+		return nil, fmt.Errorf("%w: deleted %d workloads", ErrExtraWorkloads, len(toDelete))


Actually, IIUC, this fixes a bug in logging, nice.

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2023-07-27T15:37:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, stuton

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alculquicondor]

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

Anton Stuchinskii and others added 2 commits July 12, 2023 14:42

extend unit test coverage for job controller

7dab4d0

Added two unit tests for reconciler.go

43d754a

* New tests validate that job is suspended if it's parent workload is not found, or if it's admitted.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jul 12, 2023

k8s-ci-robot requested review from tenzen-y and trasc July 12, 2023 15:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 12, 2023

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

k8s-ci-robot assigned trasc Jul 12, 2023

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 12, 2023

stuton force-pushed the extend-unit-test-coverage branch 2 times, most recently from 37507b8 to cfba333 Compare July 12, 2023 15:57

trasc reviewed Jul 14, 2023

View reviewed changes

pkg/controller/jobs/job/job_controller_test.go Outdated Show resolved Hide resolved

stuton force-pushed the extend-unit-test-coverage branch from cfba333 to 2d9ce49 Compare July 14, 2023 15:23

trasc reviewed Jul 18, 2023

View reviewed changes

pkg/controller/jobs/job/job_controller_test.go Outdated Show resolved Hide resolved

pkg/controller/jobs/job/job_controller_test.go Outdated Show resolved Hide resolved

pkg/controller/jobs/job/job_controller_test.go Show resolved Hide resolved

stuton force-pushed the extend-unit-test-coverage branch from 2d9ce49 to 057c254 Compare July 21, 2023 10:50

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 Jul 21, 2023

alculquicondor reviewed Jul 21, 2023

View reviewed changes

stuton force-pushed the extend-unit-test-coverage branch 2 times, most recently from 755849e to d89274b Compare July 24, 2023 12:46

mimowo reviewed Jul 25, 2023

View reviewed changes

pkg/controller/jobs/job/job_controller_test.go Show resolved Hide resolved

pkg/controller/jobs/job/job_controller_test.go Show resolved Hide resolved

pkg/util/testing/wrappers.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned mimowo Jul 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2023

stuton force-pushed the extend-unit-test-coverage branch from d89274b to 712aa75 Compare July 26, 2023 15:40

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2023

Merge pull request #6 from epam/extend-unit-test-coverage-achernevskii

164145d

Added two unit tests for reconciler.go

stuton force-pushed the extend-unit-test-coverage branch from 712aa75 to 164145d Compare July 27, 2023 09:09

mimowo reviewed Jul 27, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Jul 27, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2023

stuton mentioned this pull request Jul 27, 2023

check k8s version for synchronization of completions and parallelism #1014

Merged

alculquicondor reviewed Jul 27, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2023

alculquicondor added this to the v0.5 milestone Jul 27, 2023

k8s-ci-robot merged commit 24ea2ac into kubernetes-sigs:main Jul 27, 2023

k8s-ci-robot modified the milestones: v0.5, v0.4 Jul 27, 2023

stuton mentioned this pull request Oct 25, 2023

REQUEST: New membership for stuton kubernetes/org#4543

Closed

9 tasks

trasc deleted the extend-unit-test-coverage branch March 12, 2024 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend unit-test coverage for jobframework #979

Extend unit-test coverage for jobframework #979

stuton commented Jul 12, 2023

k8s-ci-robot commented Jul 12, 2023

netlify bot commented Jul 12, 2023 •

edited

Loading

stuton commented Jul 12, 2023

alculquicondor commented Jul 12, 2023

trasc left a comment

alculquicondor Jul 21, 2023

stuton Jul 24, 2023

alculquicondor Jul 21, 2023

mimowo left a comment

mimowo left a comment

mimowo Jul 27, 2023

alculquicondor left a comment

k8s-ci-robot commented Jul 27, 2023

Extend unit-test coverage for jobframework #979

Extend unit-test coverage for jobframework #979

Conversation

stuton commented Jul 12, 2023

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?

k8s-ci-robot commented Jul 12, 2023

netlify bot commented Jul 12, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

stuton commented Jul 12, 2023

alculquicondor commented Jul 12, 2023

trasc left a comment

Choose a reason for hiding this comment

alculquicondor Jul 21, 2023

Choose a reason for hiding this comment

stuton Jul 24, 2023

Choose a reason for hiding this comment

alculquicondor Jul 21, 2023

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

mimowo Jul 27, 2023

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 27, 2023

netlify bot commented Jul 12, 2023 •

edited

Loading