-
Notifications
You must be signed in to change notification settings - Fork 14
Extract common extension utils into a separate package #123
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
base: main
Are you sure you want to change the base?
Conversation
|
I think here we should be moving tests for request handling to the new package, based on the typedb ones, but using a well-known and neutral dockerised service that has grpc + http support. Then tests in the specific extensions would just need to test basic connectivity or options for those. Not immediately sure what that other test service might be. (We might also be able to test these things without running the tests inside localstack itself?) |
d7cb435 to
75f2301
Compare
|
Great point @purcell - I've now adjusted the PR to include some tests. The tests are split up into unit tests (self-contained, in-process), as well as integration tests (for the gRPC/HTTP2 proxy tests the test is using the |
| "h2", | ||
| "priority", | ||
| # TODO remove / replace prior to merge! | ||
| # "localstack-extensions-utils", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO (note to self): remove / replace prior to merge...
Move utility classes from typedb extension to a new top-level utils package that can be shared across multiple LocalStack extensions: - ProxiedDockerContainerExtension: base class for Docker-based extensions - ProxyResource: HTTP/1.1 request proxy resource - HTTP2/gRPC proxy utilities for forwarding binary traffic The new package is published as 'localstack-extensions-utils' on PyPI. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change the module structure from localstack_extensions_utils to localstack.extensions.utils to follow LocalStack naming conventions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…nsions Rename from localstack.extensions.utils to localstack_extensions.utils to avoid conflicts with the main localstack package namespace. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add Makefile with build, publish, format, and lint targets - Update README: rename Installation to Usage, show pyproject.toml dependency format Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add unit tests for HTTP/2 frame parsing and TcpForwarder (33 tests) - Add integration tests using grpcbin Docker container (19 tests) - Move test_get_frames_from_http2_stream from typedb to utils - Add test dependencies and pytest markers to pyproject.toml - Add test targets to Makefile (test, test-unit, test-integration) - Add proto files for grpcbin service definitions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add .github/workflows/utils.yml with unit and integration test jobs - Use localstack.utils.net.wait_for_port_open instead of custom implementation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixes missing jsonpatch transitive dependency when importing from localstack.utils.net in integration tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
jsonpatch is a transitive dependency of localstack that may not be installed automatically in all environments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The integration tests work at the raw HTTP/2 frame level and don't need generated gRPC stubs. This simplifies the test setup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The exports were accidentally removed, breaking imports for dependent packages like typedb. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use localstack's DOCKER_CLIENT utility for container management in integration tests for better consistency and error handling. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove redundant 'assert True' statements (no exception = success) - Update docstrings to clarify these tests validate the utility functions (TcpForwarder, frame parsing), not the LocalStack proxy integration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove test_tcp_forwarder.py (mocked unit tests not adding value) - Remove tests/conftest.py with custom marker logic - Remove marker definitions from pyproject.toml - Tests are now simply run by directory path Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace time.sleep() calls in tests with threading.Event for synchronization - Reduce fixture initialization sleep from 1.0s to 0.5s - Add parse_server_frames() helper for parsing HTTP/2 server responses - Fix first test to do proper HTTP/2 handshake instead of raw TCP connect Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| # Give server time to process | ||
| time.sleep(0.1) | ||
|
|
||
| # Connection is now established | ||
| # We've verified we can perform HTTP/2 handshake with grpcbin | ||
| assert True | ||
| finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this test really verifies anything.
utils/tests/integration/conftest.py
Outdated
| "--name", | ||
| container_name, | ||
| "-p", | ||
| f"{GRPCBIN_INSECURE_PORT}:{GRPCBIN_INSECURE_PORT}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this perhaps be using and testing the ProxiedDockerContainerExtension class rather than starting the grpcbin test server directly? In their current form these integration tests are more like network-enabled unit tests for the lower-level classes like TcpForwarder, so it's a lot of detailed tests that will be relatively brittle. I'd imagine it would be more concise and realistic to use ProxiedDockerContainerExtension and talk directly to it on the intended ports, thus covering the Docker -> proxy/TcpForwarder chain.
| # Give server time to respond | ||
| time.sleep(0.5) | ||
|
|
||
| # Verify we got data | ||
| assert len(received_data) > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this test is doing either, really.
| class TestTcpForwarderConcurrency: | ||
| """Tests for concurrent operations in TcpForwarder.""" | ||
|
|
||
| def test_multiple_sends(self, grpcbin_host, grpcbin_insecure_port): | ||
| """Test multiple sequential sends.""" | ||
| forwarder = TcpForwarder(port=grpcbin_insecure_port, host=grpcbin_host) | ||
| try: | ||
| # Send preface first | ||
| forwarder.send(HTTP2_PREFACE) | ||
| # Then settings | ||
| settings_frame = b"\x00\x00\x00\x04\x00\x00\x00\x00\x00" | ||
| forwarder.send(settings_frame) | ||
| # Then settings ACK | ||
| settings_ack = b"\x00\x00\x00\x04\x01\x00\x00\x00\x00" | ||
| forwarder.send(settings_ack) | ||
| # All sends should succeed | ||
| assert True | ||
| finally: | ||
| forwarder.close() | ||
|
|
||
| def test_concurrent_connections(self, grpcbin_host, grpcbin_insecure_port): | ||
| """Test multiple concurrent TcpForwarder connections.""" | ||
| forwarders = [] | ||
| try: | ||
| for _ in range(3): | ||
| forwarder = TcpForwarder(port=grpcbin_insecure_port, host=grpcbin_host) | ||
| forwarders.append(forwarder) | ||
|
|
||
| # All connections should be established | ||
| assert len(forwarders) == 3 | ||
|
|
||
| # Send preface to all | ||
| for forwarder in forwarders: | ||
| forwarder.send(HTTP2_PREFACE) | ||
|
|
||
| finally: | ||
| for forwarder in forwarders: | ||
| forwarder.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't really confirm that the forwarders are functional and independent.
| @@ -0,0 +1,198 @@ | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given tests like these, I definitely think the integration tests above should be much reduced, and just talk to the ProxiedDockerContainerExtension.
|
Yeah, nice! |
1250e7a to
426e040
Compare
- Add env_vars and health_check_fn parameters to ProxiedDockerContainerExtension - Refactor ParadeDbExtension to use shared base class - Improve integration tests to use ProxiedDockerContainerExtension - Remove weak test assertions and fix flaky tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Merge test_tcp_forwarder_live.py and test_grpc_connectivity.py into test_http2_proxy.py - Add test_grpc_e2e.py with actual gRPC calls to grpcbin services - Extract shared HTTP/2 constants and parse_server_frames helper to conftest.py - Reduce test code from 794 to 643 lines (19% reduction, 151 lines eliminated) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
While developing a couple of LS Extensions recently (e.g., TypeDB, Wiremock, ParadeDB), we saw some patterns occuring frequently - for example, creating an extension that wraps a running Docker container and proxies any HTTP/HTTP2/gRPC network traffic to/from it through the LS Gateway (the
ProxiedDockerContainerExtensioncan be used as a base class to abstract out the).This PR creates a new
localstack-extensions-utilspackage that can be used to externalize common extension utils and make them accessible to different extensions. The library will be pushed to pypi.org post-merge.Note 🤖 : This PR was co-created with Claude Code.