Feat/anonymous instances test op#6#65
Conversation
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_op6_schema_validation.py (1)
640-787: Consider extracting shared schema builders to reduce duplication.Both new classes repeat near-identical base/derived schema payload construction. A small helper (or fixture-like builder) will reduce drift and make future OP#6 variants easier to add.
♻️ Refactor sketch
+def _base_event_schema(schema_id: str) -> dict: + return { + "$$id": schema_id, + "$$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "required": ["id", "type", "tenantId", "occurredAt"], + "properties": { + "type": {"type": "string"}, + "id": {"type": "string", "format": "uuid"}, + "tenantId": {"type": "string", "format": "uuid"}, + "occurredAt": {"type": "string", "format": "date-time"}, + "payload": {"type": "object"}, + }, + "additionalProperties": False, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op6_schema_validation.py` around lines 640 - 787, The test files duplicate the same base/derived JSON-schema construction across TestCaseTestOp6ValidateInstance_AnonymousInstance and TestCaseTestOp6ValidateInstance_AnonymousInstance_Invalid; extract the repeated JSON payloads into reusable builder helpers (e.g., functions like build_base_event_schema() and build_derived_event_schema(type_id, payload_spec)) and call those from the teststeps instead of inlining the large dicts; update the RunRequest blocks titled "register base event schema" and "register derived event schema" to .with_json(build_base_event_schema()) and .with_json(build_derived_event_schema(...)) respectively (use the existing $$id constants such as "gts://gts.x.test6anon.events.type.v1~" and the derived ids like "gts://gts.x.test6anon.events.type.v1~x.commerce.orders.order_placed.v1.0~" so tests keep same semantics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_op6_schema_validation.py`:
- Around line 640-787: The test files duplicate the same base/derived
JSON-schema construction across
TestCaseTestOp6ValidateInstance_AnonymousInstance and
TestCaseTestOp6ValidateInstance_AnonymousInstance_Invalid; extract the repeated
JSON payloads into reusable builder helpers (e.g., functions like
build_base_event_schema() and build_derived_event_schema(type_id, payload_spec))
and call those from the teststeps instead of inlining the large dicts; update
the RunRequest blocks titled "register base event schema" and "register derived
event schema" to .with_json(build_base_event_schema()) and
.with_json(build_derived_event_schema(...)) respectively (use the existing $$id
constants such as "gts://gts.x.test6anon.events.type.v1~" and the derived ids
like "gts://gts.x.test6anon.events.type.v1~x.commerce.orders.order_placed.v1.0~"
so tests keep same semantics).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e926c43f-1c32-4b1a-8bac-2597c43092db
📒 Files selected for processing (2)
.gitignoretests/test_op6_schema_validation.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_op6_schema_validation.py (1)
187-205:⚠️ Potential issue | 🔴 CriticalUse the same instance ID for registration and validation in the invalid-instance test.
Line 189 registers
...order_placed...some_event2..., but Line 204 validates...invalid.event...some_event2.... This currently tests a lookup mismatch rather than payload-schema failure.Suggested fix
- "id": "gts.x.test6.events.type.v1~x.commerce.orders.order_placed.v1.0~x.y._.some_event2.v1.0", + "id": "gts.x.test6.events.type.v1~x.test6.invalid.event.v1.0~x.y._.some_event2.v1.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op6_schema_validation.py` around lines 187 - 205, The invalid-instance test is using two different instance IDs (the registration uses "gts.x.test6.events.type.v1~x.commerce.orders.order_placed.v1.0~x.y._.some_event2.v1.0" while the validation uses "gts.x.test6.events.type.v1~x.test6.invalid.event.v1.0~x.y._.some_event2.v1.0"), causing a lookup mismatch; update the Step with RunRequest titled "validate instance should fail" so its .with_json payload's "instance_id" exactly matches the id used in the earlier registration (the string starting with "gts.x.test6.events.type.v1~x.commerce.orders.order_placed.v1.0~x.y._.some_event2.v1.0") so the test exercises payload-schema validation rather than a missing-instance lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_op6_schema_validation.py`:
- Around line 17-32: The helper _base_event_schema currently forces the "id"
property to have format "uuid", which breaks tests that use well-known/chained
GTS IDs; update _base_event_schema to accept an argument (e.g., id_schema or
allow_well_known flag) and use that to set the "id" property schema (either
{"type":"string","format":"uuid"} or a more permissive {"type":"string"} that
allows chained GTS IDs), then update the anonymous-callsite test classes
mentioned in the file to call _base_event_schema with the permissive id option
where they use well-known IDs.
---
Outside diff comments:
In `@tests/test_op6_schema_validation.py`:
- Around line 187-205: The invalid-instance test is using two different instance
IDs (the registration uses
"gts.x.test6.events.type.v1~x.commerce.orders.order_placed.v1.0~x.y._.some_event2.v1.0"
while the validation uses
"gts.x.test6.events.type.v1~x.test6.invalid.event.v1.0~x.y._.some_event2.v1.0"),
causing a lookup mismatch; update the Step with RunRequest titled "validate
instance should fail" so its .with_json payload's "instance_id" exactly matches
the id used in the earlier registration (the string starting with
"gts.x.test6.events.type.v1~x.commerce.orders.order_placed.v1.0~x.y._.some_event2.v1.0")
so the test exercises payload-schema validation rather than a missing-instance
lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3df6ca34-e352-40cd-9e1e-10007019af9a
📒 Files selected for processing (1)
tests/test_op6_schema_validation.py
GeraBart
force-pushed
the
feat/anonymous_instances_test_OP#6
branch
from
2cbb879 to
775d237
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_op6_schema_validation.py (1)
191-191:⚠️ Potential issue | 🟠 MajorFix mismatched registered instance ID in invalid-instance test.
Line 191 registers
idunder the...order_placed...chain, but validation later targets...x.test6.invalid.event...(Line 206). This can make the test pass for the wrong reason (not found) instead of payload-schema failure.Suggested fix
- "id": "gts.x.test6.events.type.v1~x.commerce.orders.order_placed.v1.0~x.y._.some_event2.v1.0", + "id": "gts.x.test6.events.type.v1~x.test6.invalid.event.v1.0~x.y._.some_event2.v1.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op6_schema_validation.py` at line 191, The registered instance ID string assigned to variable "id" in tests/test_op6_schema_validation.py is for "...orders.order_placed..." but the invalid-instance validation later expects the "...x.test6.invalid.event..." ID; update the "id" value used in the registration to the exact instance ID that the test validates (i.e., the full "...x.test6.invalid.event..." namespaced/versioned string) so the test exercises payload-schema validation rather than a missing-instance error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_op6_schema_validation.py`:
- Line 191: The registered instance ID string assigned to variable "id" in
tests/test_op6_schema_validation.py is for "...orders.order_placed..." but the
invalid-instance validation later expects the "...x.test6.invalid.event..." ID;
update the "id" value used in the registration to the exact instance ID that the
test validates (i.e., the full "...x.test6.invalid.event..."
namespaced/versioned string) so the test exercises payload-schema validation
rather than a missing-instance error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0edaef5c-6369-4af7-912e-685d87011934
📒 Files selected for processing (1)
tests/test_op6_schema_validation.py
GeraBart
force-pushed
the
feat/anonymous_instances_test_OP#6
branch
from
775d237 to
8918215
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_op6_schema_validation.py (1)
732-790: Consider extracting shared anonymous-flow setup into a helper.Both anonymous test classes duplicate base/derived registration and instance scaffolding. A small builder/helper would reduce maintenance noise and keep intent-focused assertions in each class.
Also applies to: 806-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op6_schema_validation.py` around lines 732 - 790, Extract the duplicated anonymous-flow setup into a small helper used by both test classes: create a function (e.g., build_anonymous_flow or register_anonymous_flow) that takes the unique symbols _base_event_schema, _derived_event_schema, ORDER_PLACED_PAYLOAD_SCHEMA and returns the sequence of Step/RunRequest operations (register base, register derived, create anonymous instance, validate instance). Replace the duplicated blocks in teststeps with a call to this helper so tests keep only intent-focused assertions; ensure the helper exposes parameters for instance id/tenantId if tests need variation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_op6_schema_validation.py`:
- Around line 836-837: The test "invalid-anonymous" uses a non-RFC4122 UUID
string ("8b2e3f45-6789-4abc-0123-bcdef1234567") for the JSON "id" and appears
also for "tenantId" in the same test; replace all three occurrences with
RFC4122-compliant UUIDs (e.g., change the problematic "0123" segment) so that
format validation passes and the test can properly exercise the
missing-required-field behavior in the invalid-anonymous test case.
---
Nitpick comments:
In `@tests/test_op6_schema_validation.py`:
- Around line 732-790: Extract the duplicated anonymous-flow setup into a small
helper used by both test classes: create a function (e.g., build_anonymous_flow
or register_anonymous_flow) that takes the unique symbols _base_event_schema,
_derived_event_schema, ORDER_PLACED_PAYLOAD_SCHEMA and returns the sequence of
Step/RunRequest operations (register base, register derived, create anonymous
instance, validate instance). Replace the duplicated blocks in teststeps with a
call to this helper so tests keep only intent-focused assertions; ensure the
helper exposes parameters for instance id/tenantId if tests need variation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cf64e09-d166-41b9-a401-ff9967778c3a
📒 Files selected for processing (1)
tests/test_op6_schema_validation.py
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
GeraBart
force-pushed
the
feat/anonymous_instances_test_OP#6
branch
from
8918215 to
048994f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_op6_schema_validation.py (1)
732-790: Use instance-level copies ofteststepsto avoid shared mutable state across test runs.Class-level
teststepslists present a risk if HttpRunner or any test runner/plugin mutates the list during execution (via hooks, retries, or concurrent runs). While this framework executes steps sequentially by design, defensive copy prevents "modify-while-iterating" issues and shared state problems if execution frameworks change.♻️ Recommended pattern
+from copy import deepcopy ... class TestCaseTestOp6ValidateInstance_AnonymousInstance(HttpRunner): config = Config(...).base_url(get_gts_base_url()) def test_start(self): """Run the test steps.""" + self.teststeps = deepcopy(self.__class__.teststeps) super().test_start() class TestCaseTestOp6ValidateInstance_AnonymousInstance_Invalid(HttpRunner): config = Config(...).base_url(get_gts_base_url()) def test_start(self): """Run the test steps.""" + self.teststeps = deepcopy(self.__class__.teststeps) super().test_start()Also applies to: 806-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_op6_schema_validation.py` around lines 732 - 790, The shared class-level list teststeps is mutable and can be mutated by runners; make tests use instance-level copies by turning the static teststeps into a factory or by copying it at test runtime—e.g., provide a function (get_teststeps or similar) that returns a fresh list or call list(teststeps)/copy.deepcopy(teststeps) before passing to the runner; update references around teststeps, RunRequest, and Step in this file (and the other block at 806-858) to consume the returned/copyed list so each test run gets an independent list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_op6_schema_validation.py`:
- Around line 732-790: The shared class-level list teststeps is mutable and can
be mutated by runners; make tests use instance-level copies by turning the
static teststeps into a factory or by copying it at test runtime—e.g., provide a
function (get_teststeps or similar) that returns a fresh list or call
list(teststeps)/copy.deepcopy(teststeps) before passing to the runner; update
references around teststeps, RunRequest, and Step in this file (and the other
block at 806-858) to consume the returned/copyed list so each test run gets an
independent list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9715c44e-81df-4eff-bb7e-181ad00ef2cb
📒 Files selected for processing (1)
tests/test_op6_schema_validation.py
|
Re: deepcopy nitpick from the latest review — Skipping this one. None of the other 12 test classes in this file use |
2 participants
Summary by CodeRabbit