-
Notifications
You must be signed in to change notification settings - Fork 0
Jrepp - Readme Cleanup #203
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
User request: "create a new branch to test the taskfile testing infrastructure, if possible we should compose the taskfile from some sub task files - the integration testing should probably live in it's own taskfile in the ./testing directory - I want to have a task that will ensure the docker-compose is up and run through each integration test that runs against this set of services" Created modular testing infrastructure with: - testing/Taskfile.yml with comprehensive test management - Infrastructure lifecycle tasks (infra-up, infra-down, infra-status) - Integration test tasks with auto-managed infrastructure - Acceptance test tasks for all patterns - Backend-specific test tasks - MCP integration test tasks - Comprehensive test suites (test-all, test-all-with-mcp) - Quick smoke test for development Updated root Taskfile.yml to include testing/Taskfile.yml via includes All tasks namespaced as test-infra:* for clarity Benefits: - Separation of concerns (build vs testing infrastructure) - Easier maintenance with focused test Taskfile - Automatic infrastructure management with defer cleanup - Supports both individual and comprehensive test execution - Clear documentation in testing/README.md Co-Authored-By: Claude <[email protected]>
User request: "let's simplify the tasks so it's just test: for the namespace" Changed namespace from test-infra to test for cleaner, more intuitive usage: - Updated root Taskfile.yml includes section - Updated all documentation in testing/README.md - All testing tasks now accessible as task test:* Examples: - task test:infra-up (was test-infra:infra-up) - task test:test-all (was test-infra:test-all) - task test:help (was test-infra:help) Benefits: - Shorter, cleaner namespace - More intuitive for users - Consistent with common task naming patterns Co-Authored-By: Claude <[email protected]>
User request: "do unit tests fit into this new task namespace? yes" Moved ALL test tasks from root Taskfile.yml to testing/Taskfile.yml under unified test: namespace with clear hierarchy: Unit Tests (Fast, No Dependencies): - unit-all, unit-proxy, unit-core, unit-prismctl, unit-operator - unit-drivers (memstore, redis, nats, kafka, postgres) Integration Tests (Requires Infrastructure): - test-integration-all, integration-rust, integration-k8s - test-integration-lifecycle, test-integration-dataplane, etc. Acceptance Tests: - test-acceptance-all (docker-compose based) - acceptance-testcontainers (testcontainers based) - acceptance-parallel Parallel Execution: - parallel, parallel-fast, parallel-fail-fast Comprehensive Suites: - all (unit + integration + acceptance) - all-with-mcp Changes: - Added all unit tests to testing/Taskfile.yml - Added testcontainers-based acceptance tests - Added Rust and K8s integration tests - Added parallel execution tasks - Removed all 250+ lines of test tasks from root Taskfile.yml - Updated comprehensive suites to include unit tests - Updated testing/README.md with new structure and hierarchy - Updated help task with complete test organization Total: 55 test tasks now consolidated under test: namespace Benefits: - Single namespace for ALL tests (unit, integration, acceptance) - Clear hierarchy: Unit -> Integration -> Acceptance -> All - Fast iteration: Unit tests require no infrastructure - Easy discovery: task --list | grep test: - Consistent naming: Predictable patterns throughout - Root Taskfile focused on builds, not tests Co-Authored-By: Claude <[email protected]>
User request: "the information priority is that we want to have a very very high level mermaid diagram with a single line of text that encapsulates the power of the prism data layer after the diagram we want to describe 3 key features with short value based differentiators, then a quickstart command example with the local stack and a grpcurl output from key/val then the badges and the rest of the readme but we should keep the readme very focused on time to value by the mermaid diagram we should include links to the documentation main sections, user guide, intro, architectures, rfc - the information priority should naturally lead a reader to discover more depth and value" User request: "use the key documents and adrs and a competitive analysis to understand the key differentiators" User request: "calling out the netflix inspiration is also good I think to help people understand the focus" Changes: - Add Netflix inspiration tagline prominently at top - Simplify architecture diagram with clickable docs links - Replace generic features with 3 data-driven differentiators: * 10-100x performance (16x P50 latency, 10x throughput, 25x less memory) * Self-service infrastructure (days → minutes provisioning) * Real backends testing (Docker-based, sub-second feedback) - Add quickstart with task commands and grpcurl KeyValue example - Move badges after quickstart (value-first approach) - Add competitive comparison table vs Netflix Data Gateway - Highlight: "Netflix proved architecture at scale. Prism makes it accessible" All numbers sourced from ADR-001 (Rust performance), ADR-002 (client config), and ADR-004 (local testing). Information hierarchy optimized for time-to-value. Co-Authored-By: Claude <[email protected]>
|
This PR has merge conflicts with the base branch. Please resolve them. |
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.
Pull Request Overview
This PR restructures the testing infrastructure by consolidating all test-related tasks into a dedicated test: namespace, improving organization and discoverability. The changes include creating a new testing directory with comprehensive documentation, moving all test tasks from the root Taskfile to a dedicated testing Taskfile, and enhancing the README with a more compelling introduction that better positions Prism's value proposition.
Key Changes:
- Created
testing/Taskfile.ymlwith organized test hierarchy (unit, integration, acceptance) - Moved 30+ test tasks from root Taskfile to dedicated test namespace
- Added comprehensive testing documentation in
testing/README.md - Enhanced main README with stronger positioning and quickstart example
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| testing/Taskfile.yml | New comprehensive test infrastructure with 50+ tasks organized by test type |
| testing/README.md | Complete testing guide with examples, architecture, and troubleshooting |
| Taskfile.yml | Removed test tasks, added test namespace include, added migration note |
| README.md | Enhanced introduction, reorganized content, added quickstart example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Key Differentiators | ||
|
|
||
| ### ⚡ 10-100x Better Performance |
Copilot
AI
Nov 20, 2025
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.
[nitpick] The performance claim '10-100x Better Performance' is vague without specific context. The detailed comparison table below provides concrete metrics (16x latency, 10x throughput), but the heading lacks precision. Consider revising to '10-100x Performance Improvement Over JVM Alternatives' or referencing the specific metrics in the heading.
| ### ⚡ 10-100x Better Performance | |
| ### ⚡ 10-100x Performance Improvement Over JVM Alternatives |
| **[📖 Documentation](https://jrepp.github.io/prism-data-layer/)** | [Architecture](https://jrepp.github.io/prism-data-layer/docs/intro) | [ADRs](https://jrepp.github.io/prism-data-layer/adr) | [RFCs](https://jrepp.github.io/prism-data-layer/rfc) | ||
| --- | ||
|
|
||
| ## What is Prism? |
Copilot
AI
Nov 20, 2025
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.
[nitpick] The 'Detailed Architecture' section (lines 121-171) appears between 'What is Prism?' and 'Project Structure' sections, but the heading 'Detailed Architecture' comes after significant README content. This creates a confusing flow where readers encounter detailed diagrams before understanding the project structure. Consider moving 'Detailed Architecture' to after 'Project Structure' or renaming it to clarify its relationship to the earlier architecture overview.
| style KAFKA fill:#9f6,stroke:#333,stroke-width:2px | ||
| style POSTGRES fill:#9f6,stroke:#333,stroke-width:2px | ||
| ``` | ||
|
|
Copilot
AI
Nov 20, 2025
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.
[nitpick] The 'Detailed Architecture' section (lines 121-171) appears between 'What is Prism?' and 'Project Structure' sections, but the heading 'Detailed Architecture' comes after significant README content. This creates a confusing flow where readers encounter detailed diagrams before understanding the project structure. Consider moving 'Detailed Architecture' to after 'Project Structure' or renaming it to clarify its relationship to the earlier architecture overview.
| - echo "Running all integration tests..." | ||
| - cd {{.INTEGRATION_TESTS}} && go test -v -timeout 10m ./... | ||
| - defer: { task: infra-down } |
Copilot
AI
Nov 20, 2025
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.
The defer statement on line 289 appears after the test execution commands rather than before them. In Task's execution model, defer blocks should be placed at the beginning of the cmds list to ensure they execute even if earlier commands fail. Move this defer to the beginning of the cmds array.
| - echo "Running all integration tests..." | |
| - cd {{.INTEGRATION_TESTS}} && go test -v -timeout 10m ./... | |
| - defer: { task: infra-down } | |
| - defer: { task: infra-down } | |
| - echo "Running all integration tests..." | |
| - cd {{.INTEGRATION_TESTS}} && go test -v -timeout 10m ./... |
| - echo "Running all backend tests..." | ||
| - task: test-backend-memstore | ||
| - task: test-backend-redis | ||
| - defer: { task: infra-down } |
Copilot
AI
Nov 20, 2025
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.
Multiple tasks (test-integration-all, test-backends-all, all) use 'defer: { task: infra-down }' at the end of their command lists. These defer statements should be moved to the beginning of the cmds arrays to ensure cleanup happens even when tests fail.
| - echo "Running all backend tests..." | |
| - task: test-backend-memstore | |
| - task: test-backend-redis | |
| - defer: { task: infra-down } | |
| - defer: { task: infra-down } | |
| - echo "Running all backend tests..." | |
| - task: test-backend-memstore | |
| - task: test-backend-redis |
| - task: test-acceptance-producer | ||
| - task: test-acceptance-consumer | ||
| - task: test-acceptance-unified | ||
| - defer: { task: infra-down } |
Copilot
AI
Nov 20, 2025
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.
Multiple tasks (test-integration-all, test-backends-all, all) use 'defer: { task: infra-down }' at the end of their command lists. These defer statements should be moved to the beginning of the cmds arrays to ensure cleanup happens even when tests fail.
| - task: test-acceptance-all | ||
| - task: test-backends-all | ||
| - task: acceptance-testcontainers | ||
| - defer: { task: infra-down } |
Copilot
AI
Nov 20, 2025
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.
Multiple tasks (test-integration-all, test-backends-all, all) use 'defer: { task: infra-down }' at the end of their command lists. These defer statements should be moved to the beginning of the cmds arrays to ensure cleanup happens even when tests fail.
| localhost:50051 prism.v1.KeyValueService/Set | ||
|
|
||
| # Get the value back | ||
| grpcurl -plaintext \ | ||
| -d '{"namespace": "demo", "key": "greeting"}' \ | ||
| localhost:50051 prism.v1.KeyValueService/Get |
Copilot
AI
Nov 20, 2025
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.
The quickstart example references port 50051, but the infrastructure setup in testing/Taskfile.yml (line 179) indicates the Prism Proxy runs on port 8980. This port mismatch will cause the example to fail. Update the port to 8980 or clarify which service is on which port.
| localhost:50051 prism.v1.KeyValueService/Set | |
| # Get the value back | |
| grpcurl -plaintext \ | |
| -d '{"namespace": "demo", "key": "greeting"}' \ | |
| localhost:50051 prism.v1.KeyValueService/Get | |
| localhost:8980 prism.v1.KeyValueService/Set | |
| # Get the value back | |
| grpcurl -plaintext \ | |
| -d '{"namespace": "demo", "key": "greeting"}' \ | |
| localhost:8980 prism.v1.KeyValueService/Get |
|
This PR has been inactive for 14 days. Please update it or close it if it's no longer needed. |
User request: "look at all local branches for unmerged commits, create PRs if they are found by first merging origin/main and submitting the commit data"
This branch contains 4 unmerged commit(s). Conflicts resolved automatically with aggressive strategy.
Co-Authored-By: Claude [email protected]