test: port missing searchable JSON tests to stack package#328
Open
test: port missing searchable JSON tests to stack package#328
Conversation
📝 WalkthroughWalkthroughAdds ~1,100 lines of PostgreSQL JSONB integration tests, updates CI to run Node matrix and a new Bun-based test job, and raises the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Runner
participant Node as Node Setup (matrix)
participant Bun as Bun Setup
participant PNPM as pnpm
participant Build as Turbo Build
participant Vitest as vitest/bunx
GH->>Runner: push / PR triggers workflow
alt Node matrix job (22,24)
Runner->>Node: setup node (matrix.node-version)
Runner->>PNPM: install deps (pnpm)
Runner->>Build: pnpm turbo build --filter './packages/*'
Runner->>Vitest: run vitest (node)
end
alt Bun job
Runner->>Bun: install/setup Bun
Runner->>PNPM: install deps (pnpm)
Runner->>Build: pnpm turbo build --filter './packages/*'
Runner->>Vitest: bunx --bun vitest run (selected packages) || true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Port 48 integration tests covering array operations, containment, field access, path queries, and equality comparisons. These tests were only in @cipherstash/protect but not in @cipherstash/stack, which allowed the array_index_mode regression to go undetected.
coderdan
force-pushed
the
feat/port-searchable-json-tests
branch
from
a0849a4 to
aa5a2d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stack/__tests__/searchable-json-pg.test.ts (2)
1356-1417: Inconsistent SQL query pattern in Simple variants.These Simple tests use string interpolation (
'${selectorTerm}','${TEST_RUN_ID}') while other Simple tests in this PR (e.g., contained-by at lines 1229-1284) use parameterized queries ($1,$2). Consider aligning to parameterized queries for consistency within the new tests.Example refactor for line 1362-1367
- const rows = await sql.unsafe( - `SELECT id, (metadata).data as metadata - FROM "protect-ci-jsonb" t - WHERE eql_v2.jsonb_path_query_first(t.metadata, '${selectorTerm}'::eql_v2_encrypted) IS NOT NULL - AND t.test_run_id = '${TEST_RUN_ID}'`, - ) + const rows = await sql.unsafe( + `SELECT id, (metadata).data as metadata + FROM "protect-ci-jsonb" t + WHERE eql_v2.jsonb_path_query_first(t.metadata, $1::eql_v2_encrypted) IS NOT NULL + AND t.test_run_id = $2`, + [selectorTerm, TEST_RUN_ID], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/__tests__/searchable-json-pg.test.ts` around lines 1356 - 1417, The new "Simple" tests (e.g., the it blocks titled 'finds row by string field (Simple)', 'finds row by nested path (Simple)', and 'returns no rows for unknown path (Simple)') interpolate selectorTerm and TEST_RUN_ID directly into sql.unsafe instead of using parameterized placeholders like other tests; update those sql.unsafe calls to use parameterized queries ($1, $2) and pass selectorTerm and TEST_RUN_ID as parameters to avoid SQL injection and match the pattern used in the contained-by tests, keeping encryptQueryTerm, selectorTerm, sql.unsafe, and TEST_RUN_ID as the referenced symbols to change.
1580-1601: Testing implementation internals.This test reaches into internal STE vec structure (
eql_v2.selector(),eql_v2.is_ste_vec_array(),eql_v2.ste_vec()) to verify[@]selector behavior. While this may be necessary to detect thearray_index_modedivergence mentioned in the PR objectives, consider whether this internal structure is stable or if there's a public API approach.As per coding guidelines: "Prefer testing via the public API and avoid reaching into private internals".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/__tests__/searchable-json-pg.test.ts` around lines 1580 - 1601, The test is inspecting private STE internals (eql_v2.selector, eql_v2.is_ste_vec_array, eql_v2.ste_vec) to assert the `[@]` selector; instead, rewrite the test to exercise the public API that consumes encrypted selectors (use the existing helper encryptQueryTerm and the public search/query endpoint or SQL function that takes the encrypted selector) so you assert behavior via the public search result rather than by unnesting STE internals. Concretely: remove the LATERAL unnest of eql_v2.ste_vec and the calls to eql_v2.selector/is_ste_vec_array, call encryptQueryTerm('$.colors[@]', 'steVecSelector') (as done with selectorAt), then use the public query function or endpoint that accepts that encrypted selector (instead of selecting eql_v2.selector(selectorAt)) and assert the returned row(s) include the inserted id/marker; keep helper names (encryptQueryTerm, steVecSelector, selectorAt, hashAt) but route assertions through the public search API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/stack/__tests__/searchable-json-pg.test.ts`:
- Around line 1356-1417: The new "Simple" tests (e.g., the it blocks titled
'finds row by string field (Simple)', 'finds row by nested path (Simple)', and
'returns no rows for unknown path (Simple)') interpolate selectorTerm and
TEST_RUN_ID directly into sql.unsafe instead of using parameterized placeholders
like other tests; update those sql.unsafe calls to use parameterized queries
($1, $2) and pass selectorTerm and TEST_RUN_ID as parameters to avoid SQL
injection and match the pattern used in the contained-by tests, keeping
encryptQueryTerm, selectorTerm, sql.unsafe, and TEST_RUN_ID as the referenced
symbols to change.
- Around line 1580-1601: The test is inspecting private STE internals
(eql_v2.selector, eql_v2.is_ste_vec_array, eql_v2.ste_vec) to assert the `[@]`
selector; instead, rewrite the test to exercise the public API that consumes
encrypted selectors (use the existing helper encryptQueryTerm and the public
search/query endpoint or SQL function that takes the encrypted selector) so you
assert behavior via the public search result rather than by unnesting STE
internals. Concretely: remove the LATERAL unnest of eql_v2.ste_vec and the calls
to eql_v2.selector/is_ste_vec_array, call encryptQueryTerm('$.colors[@]',
'steVecSelector') (as done with selectorAt), then use the public query function
or endpoint that accepts that encrypted selector (instead of selecting
eql_v2.selector(selectorAt)) and assert the returned row(s) include the inserted
id/marker; keep helper names (encryptQueryTerm, steVecSelector, selectorAt,
hashAt) but route assertions through the public search API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6f6143c-8af9-4231-a6bb-f3e7a8de4bae
📒 Files selected for processing (1)
packages/stack/__tests__/searchable-json-pg.test.ts
Update @cipherstash/stack engine requirement from >=18 to >=22 to match the root package.json. Add a Node version matrix (22, 24) to CI so tests run against both current LTS and latest.
Add a non-blocking CI job that runs tests under Bun's runtime using bunx --bun vitest. Uses continue-on-error so failures don't block merges while we assess Bun compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/tests.yml (3)
15-17: Consider settingfail-fast: falsefor the matrix.By default, GitHub Actions cancels remaining matrix jobs when one fails. If you want to see test results for all Node versions regardless of individual failures, add
fail-fast: false.♻️ Suggested change
strategy: + fail-fast: false matrix: node-version: [22, 24]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 15 - 17, Add fail-fast: false to the workflow strategy so matrix jobs for node-version (the strategy.matrix.node-version matrix) won't cancel remaining runs when one job fails; update the strategy block to include fail-fast: false under the existing strategy configuration.
164-171: Improve failure visibility in the test loop.Using
|| truesilently swallows all failures, making it hard to distinguish passed tests from failed ones or missing vitest configs. Consider capturing exit codes and reporting a summary, or letting individual test runs fail while relying oncontinue-on-error: trueat the job level.♻️ Suggested improvement
- name: Run tests with Bun run: | + failed="" for dir in packages/schema packages/protect packages/stack packages/protect-dynamodb packages/drizzle packages/stack-forge; do - if [ -f "$dir/vitest.config.ts" ] || [ -f "$dir/package.json" ]; then + if [ -f "$dir/vitest.config.ts" ]; then echo "--- Testing $dir ---" - (cd "$dir" && bunx --bun vitest run) || true + (cd "$dir" && bunx --bun vitest run) || failed="$failed $dir" fi done + if [ -n "$failed" ]; then + echo "::warning::Tests failed in:$failed" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 164 - 171, The test loop currently silences all failures with "|| true" which hides failing tests; update the loop around the bunx --bun vitest run invocation to capture each run's exit code (e.g., check the exit status after the bunx --bun vitest run in the for-loop), record failing directories (refer to the loop iterating over packages/schema packages/protect ... and the bunx --bun vitest run command), print an explicit per-directory pass/fail message, and at the end print a summary and exit non-zero if any tests failed (or rely on the job-level continue-on-error but still surface per-run failures). Ensure you remove "|| true" and add logic to collect and report exit codes for visibility.
113-158: Consider extracting .env setup to a composite action.The .env file creation is duplicated across both jobs. Extracting to a composite action would reduce duplication and maintenance overhead. This is optional and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 113 - 158, Create a reusable composite action to centralize the repeated .env creation logic (extract the echo/touch sequence into a composite action, e.g., .github/actions/create-env with inputs like package_path and optional env keys) and then replace each duplicated step named "Create .env file in ./packages/protect/", "Create .env file in ./packages/stack/", "Create .env file in ./packages/protect-dynamodb/", and "Create .env file in ./packages/drizzle/" with a single uses: call to that composite action passing package_path (packages/protect, packages/stack, packages/protect-dynamodb, packages/drizzle) and any environment-specific inputs; ensure the composite action writes the same variables (CS_WORKSPACE_CRN, CS_CLIENT_ID, CS_CLIENT_KEY, CS_CLIENT_ACCESS_KEY, SUPABASE_URL, SUPABASE_ANON_KEY, DATABASE_URL, CS_ZEROKMS_HOST, CS_CTS_HOST) conditionally where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 94-95: Update the GitHub Actions checkout step to use the latest
action version: replace the uses value "actions/checkout@v3" in the step named
"Checkout Repo" with "actions/checkout@v4" so the workflow uses the v4 checkout
action.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 15-17: Add fail-fast: false to the workflow strategy so matrix
jobs for node-version (the strategy.matrix.node-version matrix) won't cancel
remaining runs when one job fails; update the strategy block to include
fail-fast: false under the existing strategy configuration.
- Around line 164-171: The test loop currently silences all failures with "||
true" which hides failing tests; update the loop around the bunx --bun vitest
run invocation to capture each run's exit code (e.g., check the exit status
after the bunx --bun vitest run in the for-loop), record failing directories
(refer to the loop iterating over packages/schema packages/protect ... and the
bunx --bun vitest run command), print an explicit per-directory pass/fail
message, and at the end print a summary and exit non-zero if any tests failed
(or rely on the job-level continue-on-error but still surface per-run failures).
Ensure you remove "|| true" and add logic to collect and report exit codes for
visibility.
- Around line 113-158: Create a reusable composite action to centralize the
repeated .env creation logic (extract the echo/touch sequence into a composite
action, e.g., .github/actions/create-env with inputs like package_path and
optional env keys) and then replace each duplicated step named "Create .env file
in ./packages/protect/", "Create .env file in ./packages/stack/", "Create .env
file in ./packages/protect-dynamodb/", and "Create .env file in
./packages/drizzle/" with a single uses: call to that composite action passing
package_path (packages/protect, packages/stack, packages/protect-dynamodb,
packages/drizzle) and any environment-specific inputs; ensure the composite
action writes the same variables (CS_WORKSPACE_CRN, CS_CLIENT_ID, CS_CLIENT_KEY,
CS_CLIENT_ACCESS_KEY, SUPABASE_URL, SUPABASE_ANON_KEY, DATABASE_URL,
CS_ZEROKMS_HOST, CS_CTS_HOST) conditionally where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5965125b-b17c-40eb-8994-f802569ddb4f
📒 Files selected for processing (2)
.github/workflows/tests.ymlpackages/stack/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/stack/package.json
Comment on lines
+94
to
+95
| - name: Checkout Repo | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
Update to actions/checkout@v4.
actions/checkout@v3 is deprecated. Since this is new code, use v4 for consistency and to benefit from Node 20 runtime and security improvements.
🔧 Suggested fix
- name: Checkout Repo
- uses: actions/checkout@v3
+ uses: actions/checkout@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Checkout Repo | |
| uses: actions/checkout@v3 | |
| - name: Checkout Repo | |
| uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 95-95: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/tests.yml around lines 94 - 95, Update the GitHub Actions
checkout step to use the latest action version: replace the uses value
"actions/checkout@v3" in the step named "Checkout Repo" with
"actions/checkout@v4" so the workflow uses the v4 checkout action.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack:
Port missing searchable JSON tests
Port 48 integration tests from
@cipherstash/protectto@cipherstash/stackcovering searchable JSON operations that were previously missing. These tests would have caught thearray_index_moderegression where the stack schema diverged from the protect schema.Test groups added
contained-by: <@ term queries(6 tests)jsonb_path_query_first: scalar path queries(6 tests)jsonb_path_exists: boolean path queries(6 tests)jsonb_array_elements + jsonb_array_length: array queries(7 tests)containment: @> with array values(5 tests)contained-by: <@ with array values(4 tests)storage: array round-trips(2 tests)containment: operand and protocol matrix(5 tests)field access: -> operator(4 tests)WHERE comparison: = equality(4 tests)CI improvements
@cipherstash/stackengine requirement from>=18to>=22to match rootcontinue-on-error) to assess Bun runtime compatibility