search

Kong / go-pdk

Write Kong plugins in Go! 🦍
https://pkg.go.dev/github.com/Kong/go-pdk
Apache License 2.0
144 stars 48 forks source link

Fix ctx test handlers #187

Closed winslowdibona closed 4 months ago

winslowdibona commented 4 months ago

There was a bug in the initial implementation of the test handlers for shared context functionality (Introduced in #174)

The usage of bridge.WrapString diverges the testing tool from how production interaction with the context is performed. This resulted in working context handling when running the library but resulted in issues when running tests.

Using structpb.NewValue aligns the testing utility with how the library interacts with kong in production through its usage of structpb.Value

winslowdibona commented 4 months ago

@nowNick @gszr After the latest tag was released I tried utilizing the changes in my code. I noticed a discrepancy between the ability to utilize the shared context when running the library vs running the test suite in my code.

gszr commented 4 months ago

@winslowdibona would it be possible to extract a test case from your testing code?

winslowdibona commented 4 months ago

@gszr I can definitely add one if needed. However I just want to mention that these changes are to the testing utility of this library, and not changes to the functionality of the library itself. I apologize as the bug I mention is one that I introduced myself, though again it was a change to the testing utility and not the core functionality.

The only test case I can think of, is one that asserts these ctx test handlers store and retrieve as a dictionary would.

I also want to mention, this functionality only supports storage and retrieval of string values in the test utility.

I was debating filing an issue to expand this to support any type supported by kong's shared context, and submitting a PR once I have some time to write out the logic.

winslowdibona commented 4 months ago

@gszr I've added a test to verify setting/getting in the shared context works for the testing utility.

winslowdibona commented 4 months ago

@gszr @nowNick Since it wasn't much additional effort, I went ahead and expanded the shared ctx test handler functionality to support storage/retrieval of multiple types (not just strings). I've added a test case as well that verifies the storage and retrieval of strings, floats, booleans, and maps from the test shared context.

winslowdibona commented 4 months ago

@gszr Anything else needed for this PR?