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.

Sign up for GitHub

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

Jump to bottom

plugin/set.go: Add test for validation protobuf flag parsing #255

Merged
merged 3 commits into from
Jul 15, 2024
Merged

plugin/set.go: Add test for validation protobuf flag parsing #255

merged 3 commits into from
Jul 15, 2024

Conversation

nywilken
Copy link
Contributor

@nywilken nywilken commented Jun 21, 2024 •
edited
Loading

This change refactors the logic inside of parseProtobufFlag to handle the case where multiple --protobuf flags are specified, while removing unnecessary guard clauses.

In addition to the refactor tests were added to validate the behavior before and after the reactor

~>  go test ./plugin/... -v
=== RUN   TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN   TestSet
--- PASS: TestSet (0.00s)
=== RUN   TestSetProtobufArgParsing
=== RUN   TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_multiple_times
--- PASS: TestSetProtobufArgParsing (0.00s)
    --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_multiple_times (0.00s)
PASS
ok      github.com/hashicorp/packer-plugin-sdk/plugin   0.250s

@nywilken nywilken requested a review from a team as a code owner June 21, 2024 19:39
@nywilken nywilken changed the base branch from main to grpc_base June 21, 2024 19:40
@nywilken nywilken changed the title nywilken.protobuf arg tests Add test for validation protobuf flag parsing Jun 21, 2024
@nywilken nywilken changed the title Add test for validation protobuf flag parsing plugin/set.go: Add test for validation protobuf flag parsing Jun 21, 2024
nywilken added 3 commits June 21, 2024 15:46
@nywilken
Add test to validate behavior for parseProtobufFlag
3cc62de 
```
~>  go test ./plugin/... -v
=== RUN   TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN   TestSet
--- PASS: TestSet (0.00s)
=== RUN   TestSetProtobufArgParsing
=== RUN   TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
--- PASS: TestSetProtobufArgParsing (0.00s)
    --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
PASS
ok      github.com/hashicorp/packer-plugin-sdk/plugin   0.249s

```
@nywilken
Drop redundant guard cases
107f07f 
The upper index bound for a slice is cap(args) we can safely retun appended slices

```
~>  go test -count=1 ./plugin/... -v
=== RUN   TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN   TestSet
--- PASS: TestSet (0.00s)
=== RUN   TestSetProtobufArgParsing
=== RUN   TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
--- PASS: TestSetProtobufArgParsing (0.00s)
    --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
PASS
ok      github.com/hashicorp/packer-plugin-sdk/plugin   0.244s

```
@nywilken
Handle the case where --protobuf is specified multiple times
d6fecfd 
```
~>  go test ./plugin/... -v
=== RUN   TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN   TestSet
--- PASS: TestSet (0.00s)
=== RUN   TestSetProtobufArgParsing
=== RUN   TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
=== RUN   TestSetProtobufArgParsing/providing_--protobuf_multiple_times
--- PASS: TestSetProtobufArgParsing (0.00s)
    --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
    --- PASS: TestSetProtobufArgParsing/providing_--protobuf_multiple_times (0.00s)
PASS
ok      github.com/hashicorp/packer-plugin-sdk/plugin   0.250s

```
@nywilken nywilken force-pushed the nywilken.protobuf-arg-tests branch from efcb82a to d6fecfd Compare June 21, 2024 19:46
sylviamoss
sylviamoss approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

The refactoring looks good and much easier to understand. I also like the new test 🎉

lbajolet-hashicorp
lbajolet-hashicorp approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

LGTM indeed! We can fold this one into the base branch now

@lbajolet-hashicorp lbajolet-hashicorp merged commit 28e1cb0 into grpc_base Jul 15, 2024
3 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the nywilken.protobuf-arg-tests branch January 21, 2025 00:14
@lbajolet-hashicorp lbajolet-hashicorp added the tech-debt Issues and pull requests related to addressing technical debt or improving the codebase label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylviamoss sylviamoss sylviamoss approved these changes

@lbajolet-hashicorp lbajolet-hashicorp lbajolet-hashicorp approved these changes

Assignees
No one assigned
Labels
tech-debt Issues and pull requests related to addressing technical debt or improving the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nywilken @sylviamoss @lbajolet-hashicorp