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

test: add unit test for flag trigger #773

Merged
merged 9 commits into from
Feb 5, 2024
Merged

test: add unit test for flag trigger #773

merged 9 commits into from
Feb 5, 2024

Conversation

kakcy
Copy link
Contributor

@kakcy kakcy commented Feb 1, 2024

Fixes #684

This pull request primarily includes adding unit tests for FlagTrigger and changes to the Bucketeer project's "pkg/feature/api" package. This change focuses on refactoring the FeatureService structure and related methods in api.go and flag_trigger.go to use the new v2fs package for feature storage. Additionally, error handling has been simplified for several methods, and the "generateTriggerURL" method no longer returns an error.

Here are the most important changes:

Add unit test for flag trigger.

Refactoring to use v2fs package:

  • pkg/feature/api/api.go: Imported the v2fs package and its FeatureStorage and FlagTriggerStorage types. These were then added as fields to the FeatureService struct, replacing direct usage of the mysqlClient. The NewFeatureService function was also updated to initialize these new fields. [1] [2] [3]
  • pkg/feature/api/api_test.go: Similar changes were made in the test file, where mock instances of FeatureStorage and FlagTriggerStorage are now created and used in the createFeatureService and createFeatureServiceNew functions. [1] [2] [3]

Simplification of error handling:

  • pkg/feature/api/flag_trigger.go: In several methods, the error handling for generateTriggerURL has been removed, as this function no longer returns errors. Instead, the URL is directly assigned to triggerURL. Additionally, error handling in ResetFlagTrigger and DeleteFlagTrigger has been simplified to return a single error status. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Refactoring of FeatureService methods:

  • pkg/feature/api/flag_trigger.go: All methods in this file that previously created new instances of FlagTriggerStorage or FeatureStorage now use the instances stored in the FeatureService struct. This includes the GetFlagTrigger, FlagTriggerWebhook, and generateTriggerURL methods. [1] [2] [3]

@kakcy kakcy changed the title test: add unit test for flag trigger [WIP] test: add unit test for flag trigger Feb 2, 2024
@kakcy kakcy marked this pull request as ready for review February 2, 2024 07:13
Copy link
Contributor

@kentakozuka kentakozuka left a comment

Choose a reason for hiding this comment

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

Thanks!
left some suggestions :)

Comment on lines 831 to 833
if err != nil {
t.Errorf("generateTriggerURL() [full] error = %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
t.Errorf("generateTriggerURL() [full] error = %v", err)
}

Comment on lines 839 to 841
if err != nil {
t.Errorf("generateTriggerURL() [masked] error = %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
t.Errorf("generateTriggerURL() [masked] error = %v", err)
}

Comment on lines 44 to 47
ctx := createContextWithToken()
ctx = metadata.NewIncomingContext(ctx, metadata.MD{
"accept-language": []string{"ja"},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ctx := createContextWithToken()
ctx = metadata.NewIncomingContext(ctx, metadata.MD{
"accept-language": []string{"ja"},
})
ctx = metadata.NewIncomingContext(
createContextWithToken(),
metadata.MD{"accept-language": []string{"ja"}},
)

@kakcy
Copy link
Contributor Author

kakcy commented Feb 5, 2024

@kentakozuka
Thank you for your suggestions!
I adopted it because it was necessary.
dbad210
f80df54

Copy link
Contributor

@kentakozuka kentakozuka left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kakcy
Copy link
Contributor Author

kakcy commented Feb 5, 2024

Thank you for the reviews!

@kakcy kakcy merged commit 402db25 into main Feb 5, 2024
18 checks passed
@kakcy kakcy deleted the unit-test-flag-trigger branch February 5, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: add unit tests for the flag trigger api
2 participants