test: add parameter serialization matrix and format validation tests#5
Merged
test: add parameter serialization matrix and format validation tests#5
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two new Lua conformance tests (format validation and parameter serialization matrix) and updates the CI workflow to install a specific Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
t/conformance/test_param_serialization_matrix.lua (1)
11-32: Avoid mutating the caller’sparam_definsidemake_spec(Line 15).
make_speccurrently mutates input state (param_def.required = true), which can create brittle tests as this helper gets reused.♻️ Suggested refactor
local function make_spec(param_def, location) + local param = cjson.decode(cjson.encode(param_def)) local path_template if location == "path" then - path_template = "/test/{" .. param_def.name .. "}" - param_def.required = true + path_template = "/test/{" .. param.name .. "}" + param.required = true else path_template = "/test" end return cjson.encode({ @@ get = { - parameters = { param_def }, + parameters = { param }, responses = { ["200"] = { description = "OK" } }, }, }, }, }) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/conformance/test_param_serialization_matrix.lua` around lines 11 - 32, make_spec mutates the input table param_def by setting param_def.required = true; instead, create a local copy of param_def inside make_spec (e.g., local param = {} and shallow-copy fields from param_def, or use a helper shallow-copy) and set param.required = true when location == "path", then use that local param in the returned spec (parameters = { param }) so the original param_def remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@t/conformance/test_format_validation.lua`:
- Around line 44-56: The current validate_format function masks ov.compile
failures by returning nil; change it to hard-fail on compile errors (so negative
tests don't treat compile-time failures as passing). In validate_format, after
calling ov.compile(spec) (referenced here as ov.compile and local v, err),
replace the nil return with a raised error or assert that includes the compile
error (tostring(err)) and context (e.g., the format_name and spec) so test
failures show the actual compile problem before calling v:validate_request.
- Line 3: The comment stating "Covers uuid, date, date-time, uri, email, ipv4,
ipv6, hostname formats." is inaccurate because ipv6 isn't tested; either add
explicit ipv6 test cases to the test suite (mirror how ipv4/hostname/email tests
are written) or remove "ipv6" from that comment so it reflects the actual
coverage; update the comment string accordingly in the test header that
currently lists the formats to keep documentation and tests in sync.
---
Nitpick comments:
In `@t/conformance/test_param_serialization_matrix.lua`:
- Around line 11-32: make_spec mutates the input table param_def by setting
param_def.required = true; instead, create a local copy of param_def inside
make_spec (e.g., local param = {} and shallow-copy fields from param_def, or use
a helper shallow-copy) and set param.required = true when location == "path",
then use that local param in the returned spec (parameters = { param }) so the
original param_def remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fe52df0-590e-4b9c-8852-ab2a8a7947bf
📒 Files selected for processing (2)
t/conformance/test_format_validation.luat/conformance/test_param_serialization_matrix.lua
nic-6443
reviewed
Apr 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test.yml:
- Around line 41-42: The JSONSCHEMA_PATH selection uses a non-deterministic find
+ head which can pick an arbitrary match; change the script around the
JSONSCHEMA_PATH assignment so it either restricts the find to the exact expected
path (e.g., constrain to the Lua 5.1 share/lua path) or collects matches and
fails if the number of results is not exactly one, and only then assigns
JSONSCHEMA_PATH and performs the sudo cp; update the logic that references
JSONSCHEMA_PATH to exit with an error if multiple or zero matches are found to
make the overlay deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b41c787-de92-4d41-8435-849e50c1c7cd
📒 Files selected for processing (1)
.github/workflows/test.yml
nic-6443
reviewed
Apr 21, 2026
nic-6443
reviewed
Apr 21, 2026
jarvis9443
added a commit
that referenced
this pull request
Apr 21, 2026
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
2 participants
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.
Add conformance test suites for parameter serialization and format validation.
Changes
Parameter serialization matrix (
t/conformance/test_param_serialization_matrix.lua)Format validation (
t/conformance/test_format_validation.lua)api7/jsonschema#96)CI update (
.github/workflows/test.yml)Summary by CodeRabbit