Skip to content

add unit-tests#28

Merged
cheyang merged 1 commit intosgl-project:mainfrom
gujingit:tests/unit-tests
Sep 18, 2025
Merged

add unit-tests#28
cheyang merged 1 commit intosgl-project:mainfrom
gujingit:tests/unit-tests

Conversation

@gujingit
Copy link
Copy Markdown
Collaborator

@gujingit gujingit commented Sep 17, 2025 •
edited
Loading

Ⅰ. Motivation

add unit-tests

Ⅱ. Modifications

add unit-tests

Ⅲ. Does this pull request fix one issue?

NONE

Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

all

Ⅴ. Describe how to verify it

$  make test

go fmt ./...
go vet ./...
go test $(go list ./cmd/... ./internal/... ./pkg/... ) -coverprofile cover.out
ok      sigs.k8s.io/rbgs/cmd/cli        0.558s  coverage: 71.6% of statements
ok      sigs.k8s.io/rbgs/cmd/rbgs       0.446s  coverage: 6.7% of statements
ok      sigs.k8s.io/rbgs/internal/controller/workloads  1.408s  coverage: 60.7% of statements
ok      sigs.k8s.io/rbgs/pkg/dependency 0.734s  coverage: 94.4% of statements
ok      sigs.k8s.io/rbgs/pkg/discovery  1.637s  coverage: 92.7% of statements
ok      sigs.k8s.io/rbgs/pkg/reconciler 27.466s coverage: 71.1% of statements
ok      sigs.k8s.io/rbgs/pkg/scale      1.975s  coverage: 100.0% of statements
ok      sigs.k8s.io/rbgs/pkg/scheduler  2.650s  coverage: 80.0% of statements
ok      sigs.k8s.io/rbgs/pkg/utils      1.031s  coverage: 50.3% of statements
go tool cover -func=cover.out | awk '/^total:/ {print $3}'
65.2%

VI. Special notes for reviews

Checklist

  • Format your code make fmt.
  • Add unit tests or integration tests.
  • Update the documentation related to the change.

@gujingit gujingit force-pushed the tests/unit-tests branch 2 times, most recently from 2a2c793 to e2d3c65 Compare September 17, 2025 13:55
@gujingit
Copy link
Copy Markdown
Collaborator Author

gujingit commented Sep 18, 2025 •
edited
Loading

staticcheck error:

Error: pkg/utils/utils.go:53:16: client.Apply is deprecated: Use client.Client.Apply() instead.  (SA1019)

It needs to replace client.Patch() with client.Apply() in PatchObjectApplyConfiguration(). This change has a wide impact and requires testing the compatibility of client.Apply() with different Kubernetes versions. This PR only add unit tests and does not fix the deprecated issue, so static check is temporarily skipped. A TODO has been added, and the fix will be submitted in a separate PR.

cheyang
cheyang reviewed Sep 18, 2025
View reviewed changes
Comment thread pkg/utils/utils.go
var filtered []corev1.EnvVar
for _, env := range envs {
if !strings.HasPrefix(env.Name, "ROLES_") && env.Name != "RBG_GROUP_SIZE" {
if !strings.HasPrefix(env.Name, "ROLE_") && env.Name != "GROUP_NAME" {
Copy link
Copy Markdown
Collaborator

@cheyang cheyang Sep 18, 2025

Choose a reason for hiding this comment

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

Are we going to introduce this breaking change?

Copy link
Copy Markdown
Collaborator Author

@gujingit gujingit Sep 18, 2025

Choose a reason for hiding this comment

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

No, it's not a breaking change. Actually, we inject GROUP_NAME, ROLE_NAME and ROLE_INDEX in buildLocalRoleVars().

cheyang
cheyang approved these changes Sep 18, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@cheyang cheyang left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@cheyang cheyang merged commit 7dc0860 into sgl-project:main Sep 18, 2025
3 checks passed
@gujingit gujingit deleted the tests/unit-tests branch September 23, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cheyang cheyang cheyang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gujingit @cheyang