Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add fuzzer for WorkstationConfig #3410

Conversation

jasonvigil
Copy link
Collaborator

No description provided.

@yuwenma
Copy link
Collaborator

yuwenma commented Dec 18, 2024

One question about trigging the fuzz test.
cc @jingyih who's working on detecting new field.

@@ -127,8 +127,6 @@ func WorkstationConfigSpec_ToProto(mapCtx *direct.MapContext, in *krm.Workstatio
out.ReadinessChecks = direct.Slice_ToProto(mapCtx, in.ReadinessChecks, WorkstationConfig_ReadinessCheck_ToProto)
out.ReplicaZones = in.ReplicaZones

ApplyWorkstationConfigGCPDefaults(mapCtx, in, out, actual)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the right place to set the default. Can I ask why we move it to the CRUD ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This had to be moved out so that the mapping function would satisfy the interface required by the fuzz tests. Applying the defaults requires passing in the actual WorkstationConfig proto.

@@ -39,6 +40,40 @@ import (

func init() {
registry.RegisterModel(krm.WorkstationConfigGVK, NewWorkstationConfigModel)
fuzztesting.RegisterKRMFuzzer(workstationConfigFuzzer())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just notice this, could you elaborate how to run this fuzz test if add the init here but not this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be a new(er) method for setting up fuzz tests. I think it's a bit easier to use. The tests are run here

, or manually with go test -v ./pkg/fuzztesting/fuzztests/ -fuzz=FuzzAllMappers -fuzztime 60s

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I believe we have a new framework for fuzzers now. "find-missing-fields" is probably deprecated.

@acpana do we have plan to migrate old-styled fuzz tests and remove the deprecated files to avoid confusion?

@jasonvigil jasonvigil requested a review from yuwenma December 18, 2024 20:39
@jasonvigil jasonvigil force-pushed the workstationconfig-fuzzer branch from e6f3571 to f4c959d Compare December 18, 2024 22:38
@yuwenma
Copy link
Collaborator

yuwenma commented Dec 19, 2024

/lgtm
/approve
Nice! Thank you for the context!

@google-oss-prow google-oss-prow bot added the lgtm label Dec 19, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuwenma

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:

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

@google-oss-prow google-oss-prow bot merged commit b410df0 into GoogleCloudPlatform:master Dec 19, 2024
17 checks passed
@jasonvigil jasonvigil deleted the workstationconfig-fuzzer branch December 26, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants