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

Fix ctx test handlers #187

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Conversation

winslowdibona
Copy link
Contributor

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
Copy link
Contributor Author

winslowdibona commented Feb 27, 2024

@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
Copy link
Member

gszr commented Feb 27, 2024

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

@winslowdibona
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@winslowdibona
Copy link
Contributor Author

winslowdibona commented Feb 29, 2024

@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
Copy link
Contributor Author

@gszr Anything else needed for this PR?

@gszr gszr merged commit ec6dca0 into Kong:master Mar 7, 2024
2 checks passed
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.

2 participants