Add CodecError Regression Tests with wireframe_testing and ExecPlan#497
Merged
Add CodecError Regression Tests with wireframe_testing and ExecPlan#497
Conversation
Contributor
Reviewer's GuideImplements context-aware EOF error construction in the length-delimited codec and adds a comprehensive CodecError regression suite (rstest + BDD via wireframe_testing), along with an ExecPlan document and roadmap/docs updates describing and wiring the new testing pattern. Sequence diagram for EOF handling in LengthDelimitedDecoder.decode_eofsequenceDiagram
participant Upstream as UpstreamDecoderUser
participant Decoder as LengthDelimitedDecoder
participant Inner as InnerDecoder
participant Buf as BytesMut
participant Ctx as EofContext
participant Builder as build_eof_error
Upstream->>Decoder: decode_eof(Buf)
Decoder->>Buf: inspect buffer
Decoder->>Ctx: from_buffer(Buf)
Ctx-->>Decoder: EofContext
Decoder->>Inner: decode_eof(Buf)
alt Inner returns frame at EOF
Inner-->>Decoder: Ok(Some(frame))
Decoder-->>Upstream: Ok(Some(BytesMut::freeze(frame)))
else Inner reports incomplete data at EOF
Inner-->>Decoder: Ok(None)
Decoder->>Builder: build_eof_error(EofContext)
Builder-->>Decoder: io::Error(CodecError::Eof(...))
Decoder-->>Upstream: Err(io::Error)
else Inner returns error
Inner-->>Decoder: Err(io::Error)
Decoder->>Decoder: classify_error(io::Error)
alt InvalidData framing error
Decoder-->>Upstream: Err(io::Error)
else other error
Decoder->>Builder: build_eof_error(EofContext)
Builder-->>Decoder: io::Error(CodecError::Eof(...))
Decoder-->>Upstream: Err(io::Error)
end
end
Class diagram for EofContext and EOF error constructionclassDiagram
class LengthDelimitedDecoder {
+decode_eof(src: BytesMut) Result~Option~BytesMut~~
}
class EofContext {
+bytes_received: usize
+expected: Option~usize~
+from_buffer(src: BytesMut) EofContext
}
class CodecError {
+Eof(EofError)
}
class EofError {
}
class EofErrorMidHeader {
+bytes_received: usize
+header_size: usize
}
class EofErrorMidFrame {
+bytes_received: usize
+expected: usize
}
class IoError {
}
class build_eof_error {
+build_eof_error(context: EofContext) IoError
}
LengthDelimitedDecoder --> EofContext : uses
LengthDelimitedDecoder --> build_eof_error : calls
build_eof_error --> EofContext : reads
build_eof_error --> CodecError : constructs
CodecError --> EofError : wraps
EofError <|-- EofErrorMidHeader
EofError <|-- EofErrorMidFrame
CodecError --> IoError : converts
Flow diagram for CodecError regression tests using wireframe_testingflowchart TD
Dev[Developer] --> CT[cargo test]
CT --> IT[test_codec_error_regressions]
CT --> BDD[codec_error_regressions_scenarios]
subgraph IntegrationTests
IT --> WFHelpers[wireframe_testing helpers\nencode_frame, new_test_codec]
WFHelpers --> Codec[LengthDelimitedFrameCodec]
Codec --> ObsHandle[ObservabilityHandle]
ObsHandle --> Labels[Assert error_type and recovery_policy]
end
subgraph BDDScenarios
BDD --> World[codec_error_regressions world]
World --> Steps[codec_error_regressions_steps]
Steps --> Fixtures[codec_error_regressions fixtures]
Fixtures --> CodecBDD[LengthDelimitedFrameCodec]
CodecBDD --> ObsHandleBDD[ObservabilityHandle]
ObsHandleBDD --> TaxonomyAsserts[Taxonomy and EOF behaviour assertions]
end
Labels --> Outcome[CodecError taxonomy regression coverage]
TaxonomyAsserts --> Outcome
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughSummarise the change: add an ExecPlan and wireframe_testing guidance; capture EOF context in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Decoder as LengthDelimitedDecoder
participant Inner as InnerDecoder
participant Builder as EOF\ Builder
participant Observ as Observability
Client->>Decoder: send bytes / EOF
Decoder->>Decoder: EofContext::from_buffer(src)
Decoder->>Inner: inner.decode_eof()
alt Inner returns Ok(None)
Decoder->>Builder: build_eof_error(EofContext)
Builder->>Observ: record EOF classification (MidHeader / MidFrame)
Builder-->>Decoder: Err(EofError)
else Inner returns Err(e) and e != InvalidData
Decoder->>Decoder: debug log
Decoder->>Builder: build_eof_error(EofContext)
Builder->>Observ: record EOF classification
Builder-->>Decoder: Err(EofError)
else Inner returns Err(InvalidData)
Inner-->>Decoder: propagate original error
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 unit tests (beta)
📝 Coding Plan
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- There’s quite a bit of duplicated helper logic between
tests/test_codec_error_regressions.rsandtests/fixtures/codec_error_regressions.rs(e.g.encoded_default_frame,classify_eof,extract_eof_error, partial header/payload builders); consider extracting these into a shared test helper module to keep the regression logic in one place. - In
partial_payload_wireyou hard-code4for the header size when allocating capacity; usingLENGTH_HEADER_SIZEinstead would keep the helper consistent with the production codec constants.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s quite a bit of duplicated helper logic between `tests/test_codec_error_regressions.rs` and `tests/fixtures/codec_error_regressions.rs` (e.g. `encoded_default_frame`, `classify_eof`, `extract_eof_error`, partial header/payload builders); consider extracting these into a shared test helper module to keep the regression logic in one place.
- In `partial_payload_wire` you hard-code `4` for the header size when allocating capacity; using `LENGTH_HEADER_SIZE` instead would keep the helper consistent with the production codec constants.
## Individual Comments
### Comment 1
<location path="tests/test_codec_error_regressions.rs" line_range="184" />
<code_context>
+ expected_clean_close: true,
+ }
+)]
+fn taxonomy_cases_emit_expected_recovery_labels(#[case] case: TaxonomyCase) {
+ assert_eq!(case.error.error_type(), case.expected_type);
+ assert_eq!(case.error.default_recovery_policy(), case.expected_policy);
</code_context>
<issue_to_address>
**suggestion (testing):** Add taxonomy cases for MidHeader and MidFrame EOF variants to fully lock in EOF policies and flags
The `taxonomy_cases_emit_expected_recovery_labels` test currently only covers `CodecError::Eof(EofError::CleanClose)` and misses the `MidHeader` and `MidFrame` variants. Please add `TaxonomyCase` entries for `EofError::MidHeader { .. }` and `EofError::MidFrame { .. }` to assert their `error_type()`, `default_recovery_policy()`, `should_disconnect()`, and `is_clean_close()` so EOF taxonomy is fully regression-tested for all variants, independent of the EOF classification helper tests.
Suggested implementation:
```rust
)]
#[case::clean_close(
TaxonomyCase {
error: CodecError::Eof(EofError::CleanClose),
expected_type: "eof",
expected_policy: RecoveryPolicy::Disconnect,
expected_disconnect: true,
expected_clean_close: true,
}
)]
#[case::mid_header(
TaxonomyCase {
error: CodecError::Eof(EofError::MidHeader {
// TODO: fill in struct fields according to the EofError::MidHeader
// definition, for example:
// header_len: 10,
// received_len: 5,
}),
expected_type: "eof",
expected_policy: RecoveryPolicy::Disconnect,
expected_disconnect: true,
expected_clean_close: false,
}
)]
#[case::mid_frame(
TaxonomyCase {
error: CodecError::Eof(EofError::MidFrame {
// TODO: fill in struct fields according to the EofError::MidFrame
// definition, for example:
// frame_len: 1024,
// received_len: 512,
}),
expected_type: "eof",
expected_policy: RecoveryPolicy::Disconnect,
expected_disconnect: true,
expected_clean_close: false,
}
)]
fn taxonomy_cases_emit_expected_recovery_labels(#[case] case: TaxonomyCase) {
```
1. Replace the `// TODO` field initializations in `EofError::MidHeader { .. }` and `EofError::MidFrame { .. }` with the actual field names and values from your `EofError` enum definition so the code compiles.
2. If `MidHeader` or `MidFrame` carry additional context that should affect taxonomy (e.g., different policies for certain sizes), adjust the `expected_policy`, `expected_disconnect`, and `expected_clean_close` values to match the real semantics. Currently these are set to `Disconnect`, `true`, and `false` respectively, assuming mid-stream EOFs are treated as non-clean disconnect-worthy errors.
</issue_to_address>
### Comment 2
<location path="tests/fixtures/codec_error_regressions.rs" line_range="191" />
<code_context>
+ Ok(wire)
+}
+
+fn classify_eof(bytes: &[u8], consume_frame_before_eof: bool) -> TestResult<EofError> {
+ let codec = LengthDelimitedFrameCodec::new(1024);
+ let mut decoder = codec.decoder();
</code_context>
<issue_to_address>
**suggestion:** Consider a regression test around EOF with zero-length payloads or header-only frames
Current tests exercise clean close, partial header, and partial payload, but always with a non‑empty payload. Please add a case with a zero‑length frame (header present, no payload) that goes through `classify_eof` and asserts the resulting `EofError` and metrics, either here or in `tests/test_codec_error_regressions.rs`, to confirm the new EOF context logic handles empty payloads correctly.
Suggested implementation:
```rust
wire.extend_from_slice(
payload
.get(..partial_len)
.ok_or("partial payload slice should be available")?,
);
Ok(wire)
}
#[test]
fn eof_with_zero_length_payload_frame_is_classified() -> TestResult<()> {
// Construct a single frame with a valid header indicating a zero-length payload
// (header present, no payload bytes).
let mut wire = Vec::with_capacity(4);
wire.extend_from_slice(&write_network_u32(0));
// Do not consume any frame before EOF; we want EOF immediately after the header-only frame.
let eof_error = classify_eof(&wire, false)?;
// Basic sanity assertion that we got some EOF classification back.
// The exact variant/metrics should be asserted in line with the other tests.
let _ = eof_error;
Ok(())
}
fn classify_eof(bytes: &[u8], consume_frame_before_eof: bool) -> TestResult<EofError> {
```
To fully implement the intent of your review comment:
1. Strengthen the assertion in `eof_with_zero_length_payload_frame_is_classified` to match the expected `EofError` variant and associated fields, following the pattern used by the other EOF regression tests in this file (e.g., `assert_eq!(eof_error, EofError::... )` or `assert!(matches!(eof_error, EofError::...))`).
2. If you are tracking metrics (counters/gauges) in other tests, also add assertions here to verify the metric values for the zero-length frame EOF case (e.g., frame count, bytes, EOF classification labels).
3. If tests are organized in `tests/test_codec_error_regressions.rs` as well, you may want to add a higher-level integration test there that uses this fixture and asserts the observable metrics exported by the codec layer for the zero-length payload EOF scenario.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codec.rs (1)
158-165:⚠️ Potential issue | 🔴 CriticalTrack pending frame state before classifying EOF.
Line 160 only checks the external buffer. After an earlier
decode()call,
LengthDelimitedCodeccan already have consumed the length prefix and be
waiting for payload bytes whilesrcis empty, so this path returnsOk(None)
instead ofEofError::MidFrame. The same missing state means
EofContext::from_buffer()can reinterpret payload bytes as a new header on
later EOFs. Persist the pending payload length inLengthDelimitedDecoder,
clear it once a frame is emitted, and build EOF errors from that stored state
rather than the current buffer snapshot. Verify this against the
tokio_util::codec::LengthDelimitedCodecpartial-read state machine.Also applies to: 195-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/codec.rs` around lines 158 - 165, The EOF handling currently checks only the external buffer in decode_eof and calls EofContext::from_buffer, which misses the case where a previous decode() consumed the length prefix and the decoder is waiting for payload bytes; modify LengthDelimitedDecoder to store pending_payload_len (Option<usize>) when a length header is parsed, consult that stored state at the start of decode_eof (and in the EOF-paths around lines referenced) to return EofError::MidFrame when pending_payload_len.is_some(), clear pending_payload_len when a full frame is emitted in decode() (or when a frame is otherwise completed), and build EOF errors from this stored pending_payload_len instead of relying solely on EofContext::from_buffer so partial-read state matches tokio_util::codec::LengthDelimitedCodec behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md`:
- Around line 339-343: Remove the hard-coded MDLINT environment override in the
recipe line that currently sets "MDLINT=/root/.bun/bin/markdownlint-cli2" and
instead invoke the documented project target "make markdownlint" so the step
runs in a normal checkout; update the line that references the MDLINT variable
to call only "make markdownlint" while keeping the surrounding set -o pipefail
and tee redirection unchanged.
In `@docs/roadmap.md`:
- Around line 440-441: Update the roadmap entry text that currently reads
"9.7.4. Add regression tests in `wireframe_testing`..." so it does not point
readers to the wrong location; change the wording to indicate the tests are
"backed by `wireframe_testing`" (or similar), e.g. replace the phrase "in
`wireframe_testing`" with "backed by `wireframe_testing`" in the line starting
with "9.7.4. Add regression tests" so the item correctly reflects that the
executable regressions live under the repository-root tests/ directory while
using wireframe_testing as a dev-dependency.
In `@tests/fixtures/codec_error_regressions.rs`:
- Around line 20-21: The public fixture surface (symbols like TestResult,
CodecErrorRegressionsWorld, the fixture constructor and its public world
methods) is undocumented and likely should not be broadly exported; either add
concise rustdoc comments (///) to each exported item explaining purpose and
usage or restrict visibility to pub(crate) for TestResult,
CodecErrorRegressionsWorld, the fixture constructor and any public methods on
the world struct; update the declarations accordingly so only intended items
remain public or are documented with /// lines describing the API.
In `@tests/test_codec_error_regressions.rs`:
- Around line 257-273: The test should not rely only on Display text—downcast
the decoding failure returned by decode_frames_with_codec in
wireframe_testing_fixture_cases_continue_to_surface_expected_errors to the
concrete wireframe::codec::CodecError and assert the expected enum variant and
its recovery policy; specifically, after expecting an error from
decode_frames_with_codec(&codec, wire), get the underlying source (e.g.,
error.source() or error.get_ref()),
downcast_ref::<wireframe::codec::CodecError>(), then match or assert that it is
the exact variant (e.g., the oversize variant vs the truncated-header variant)
and that its recovery/policy field has the expected value.
- Around line 32-95: Move the duplicated test helpers (extract_eof_error,
classify_eof, encoded_default_frame, truncated_payload_wire, record_codec_error
and the StrictRecoveryHook) into a single shared test module and replace the
local copies in both tests with imports; create rstest fixtures in that shared
module for common setup (e.g., a codec/decoder fixture used by classify_eof) and
export the helper functions so both the BDD tests and integration tests use
them, then remove the duplicated definitions from
tests/test_codec_error_regressions.rs and
tests/fixtures/codec_error_regressions.rs and update their uses to call the
shared helpers.
---
Outside diff comments:
In `@src/codec.rs`:
- Around line 158-165: The EOF handling currently checks only the external
buffer in decode_eof and calls EofContext::from_buffer, which misses the case
where a previous decode() consumed the length prefix and the decoder is waiting
for payload bytes; modify LengthDelimitedDecoder to store pending_payload_len
(Option<usize>) when a length header is parsed, consult that stored state at the
start of decode_eof (and in the EOF-paths around lines referenced) to return
EofError::MidFrame when pending_payload_len.is_some(), clear pending_payload_len
when a full frame is emitted in decode() (or when a frame is otherwise
completed), and build EOF errors from this stored pending_payload_len instead of
relying solely on EofContext::from_buffer so partial-read state matches
tokio_util::codec::LengthDelimitedCodec behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39e0fa32-6c37-4bff-a98a-20083011fff4
📒 Files selected for processing (13)
docs/execplans/11-1-2-structured-logging-and-tracing-spans.mddocs/execplans/9-7-4-codec-error-taxonomy-regression-tests.mddocs/roadmap.mddocs/wireframe-testing-crate.mdsrc/codec.rstests/features/codec_error_regressions.featuretests/fixtures/codec_error_regressions.rstests/fixtures/mod.rstests/scenarios/codec_error_regressions_scenarios.rstests/scenarios/mod.rstests/steps/codec_error_regressions_steps.rstests/steps/mod.rstests/test_codec_error_regressions.rs
Add a comprehensive execution plan document 9-7-4-codec-error-taxonomy-regression-tests.md detailing the purpose, constraints, testing strategy, risks, progress checkpoints, decision log, and expected outcomes for the 9.7.4 roadmap item. This plan establishes the approach to hardening codec-error taxonomy regression tests using the wireframe_testing crate, including staged work for regression coverage, helper improvements, BDD scenarios, documentation updates, and validation steps. This document will guide the subsequent implementation and test additions to ensure robust, reusable, and observable codec-error regression testing aligned with roadmap 9.7.4 requirements. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…codec error taxonomy and recovery - Added extensive regression tests under tests/test_codec_error_regressions.rs using rstest - Introduced BDD feature file tests/features/codec_error_regressions.feature with multiple scenarios - Implemented a new test fixture world in tests/fixtures/codec_error_regressions.rs supporting detailed EOF and recovery policy classification - Added scenario definitions in tests/scenarios/codec_error_regressions_scenarios.rs - Added step definitions in tests/steps/codec_error_regressions_steps.rs for BDD scenarios - Updated tests/fixtures/mod.rs, tests/scenarios/mod.rs, and tests/steps/mod.rs to include these new modules - Modified codec.rs decoding to preserve detailed EOF error context for accurate testing - Completed ExecPlan in docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md marking item done - Updated docs/wireframe-testing-crate.md and docs/roadmap.md to reflect test coverage and best practices This commit completes roadmap item 9.7.4 by establishing a robust regression suite that verifies codec error classification, recovery policies, observability labels, and supports future maintenance with no change to public APIs. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub
Bot
force-pushed
the
add-codecerror-regression-tests-qmk3tf
branch
from
c5552f7 to
0414c9d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
tests/test_codec_error_regressions.rs (2)
32-95: 🧹 Nitpick | 🔵 TrivialExtract duplicated codec test helpers into shared test infrastructure.
The helpers
extract_eof_error,classify_eof,encoded_default_frame,truncated_payload_wire, andStrictRecoveryHookare duplicated between this file andtests/fixtures/codec_error_regressions.rs. Move them to a shared test module to prevent drift when codec behaviour changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codec_error_regressions.rs` around lines 32 - 95, The test helpers extract_eof_error, classify_eof, encoded_default_frame, truncated_payload_wire and the StrictRecoveryHook are duplicated across tests; factor them into a shared test utility module (e.g., tests/utils/codec_helpers or a new tests::fixtures module) and update both tests/test_codec_error_regressions.rs and tests/fixtures/codec_error_regressions.rs to import the helpers from that shared module; ensure you move any dependent helpers (e.g., new_test_codec, encode_frame, LENGTH_HEADER_SIZE, write_network_u32) or re-export them as needed and adjust visibility (pub(crate)) so functions like extract_eof_error, classify_eof, encoded_default_frame, truncated_payload_wire and StrictRecoveryHook compile without changes to their callers.
257-273: 🧹 Nitpick | 🔵 TrivialAssert structured codec taxonomy on the real decode path.
These cases drive bytes through
decode_frames_with_codecbut only matchDisplayfragments. If the decoder changes theCodecErrorvariant while keeping a similar message, this regression suite still passes. Downcast the cause and assert the error type and policy directly.Note: The
decode_frames_with_codechelper erases typed errors toio::Errortext, so typed assertions require calling the decoder directly. Track this as a follow-up improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codec_error_regressions.rs` around lines 257 - 273, The test currently only checks error Display fragments from decode_frames_with_codec; change it to assert the concrete codec error by invoking the decoder directly (use wireframe::codec::examples::HotlineFrameCodec::new(...) and call its decode method instead of the helper) and downcast the returned error to the codec's error type (CodecError) then match the specific variant(s) (e.g., PayloadTooLarge / TruncatedHeader or the variant names used by your CodecError) and assert the associated fields like max_frame_length or bytes remaining; replace the string-contains assertion with these typed assertions so the test fails if the error variant/policy changes.docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md (2)
339-343:⚠️ Potential issue | 🟡 MinorRemove the absolute
MDLINTpath from the validation recipe.The plan should be runnable from a normal checkout. Hard-coding
/root/.bun/bin/markdownlint-cli2makes this step environment-specific and diverges from the documented project command.✏️ Suggested fix
-set -o pipefail && make markdownlint MDLINT=/root/.bun/bin/markdownlint-cli2 2>&1 | tee /tmp/9-7-4-markdownlint.log +set -o pipefail && make markdownlint 2>&1 | tee /tmp/9-7-4-markdownlint.log🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md` around lines 339 - 343, The recipe hard-codes an absolute MDLINT path (/root/.bun/bin/markdownlint-cli2) making the step environment-specific; update the markdownlint invocation to remove the absolute path and rely on the MDLINT variable or system PATH instead (e.g., use MDLINT=markdownlint-cli2 or omit MDLINT so the Makefile falls back to its default). Edit the line that sets MDLINT in the command for the markdownlint step so it no longer embeds /root/.bun/bin/markdownlint-cli2, ensuring the recipe runs from a normal checkout and uses the project's documented command.
193-193:⚠️ Potential issue | 🟡 MinorRemove the absolute
MDLINTpath from the validation recipe.Line 193 hard-codes
/root/.bun/bin/markdownlint-cli2, making this step environment-specific. Use the project's standardmake markdownlintinvocation instead.✏️ Suggested fix
-- `make markdownlint MDLINT=/root/.bun/bin/markdownlint-cli2` +- `make markdownlint`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md` at line 193, The recipe currently hard-codes the absolute MDLINT path (`/root/.bun/bin/markdownlint-cli2`) in the validation step; update the docs to remove that absolute path and show the project-standard invocation (`make markdownlint`) (or a generic `make markdownlint MDLINT=...` if you want to override) so the step is environment-agnostic—replace the line containing the hard-coded `MDLINT=/root/.bun/bin/markdownlint-cli2` with the plain `make markdownlint` invocation.docs/roadmap.md (1)
440-441:⚠️ Potential issue | 🟡 MinorUpdate the roadmap wording to reflect the actual test location.
The ExecPlan explicitly states that executable regressions live under the repository-root
tests/directory becausewireframe_testingis only a dev-dependency. The phrase "inwireframe_testing" misdirects readers. Replace with "backed bywireframe_testing" or similar.✏️ Suggested wording
-- [x] 9.7.4. Add regression tests in `wireframe_testing` for the `CodecError` - taxonomy and recovery policy behaviours defined in 9.1.2. Requires 9.1.2. +- [x] 9.7.4. Add regression tests backed by `wireframe_testing` for the + `CodecError` taxonomy and recovery policy behaviours defined in 9.1.2. + Requires 9.1.2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap.md` around lines 440 - 441, Update the roadmap checklist item that reads "9.7.4. Add regression tests in `wireframe_testing`..." to avoid implying tests live under `wireframe_testing`; change the phrase "in `wireframe_testing`" to "backed by `wireframe_testing`" (or similar wording) so the item communicates that regressions under the repository-root `tests/` directory are supported by the `wireframe_testing` dev-dependency; locate the string "9.7.4. Add regression tests in `wireframe_testing` for the `CodecError` taxonomy and recovery policy behaviours defined in 9.1.2. Requires 9.1.2." and edit it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md`:
- Line 257: The phrase "hand-written tests" should be corrected to the
single-word adjective "handwritten" in the documentation line that reads "Use
`#[rstest]` parameterization instead of repeated hand-written tests."; update
that sentence to "Use `#[rstest]` parameterization instead of repeated
handwritten tests." and ensure any other occurrences of "hand-written" in this
document are replaced with "handwritten" for consistent spelling.
In `@tests/fixtures/codec_error_regressions.rs`:
- Around line 132-144: Extract the duplicated match logic into a small helper
like a function that takes (expected: PartialEofKind, actual: &EofError) -> bool
(or returns Result/Unit) and use it from assert_partial_kind and the
classify_partial_inputs test; replace the inline match in assert_partial_kind
(and the match at lines 241-244 in classify_partial_inputs) with a call to that
helper, keeping the same pattern arms (PartialEofKind::MidHeader <->
EofError::MidHeader { .. } and PartialEofKind::MidFrame <-> EofError::MidFrame {
.. }) so behavior is unchanged.
---
Duplicate comments:
In `@docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.md`:
- Around line 339-343: The recipe hard-codes an absolute MDLINT path
(/root/.bun/bin/markdownlint-cli2) making the step environment-specific; update
the markdownlint invocation to remove the absolute path and rely on the MDLINT
variable or system PATH instead (e.g., use MDLINT=markdownlint-cli2 or omit
MDLINT so the Makefile falls back to its default). Edit the line that sets
MDLINT in the command for the markdownlint step so it no longer embeds
/root/.bun/bin/markdownlint-cli2, ensuring the recipe runs from a normal
checkout and uses the project's documented command.
- Line 193: The recipe currently hard-codes the absolute MDLINT path
(`/root/.bun/bin/markdownlint-cli2`) in the validation step; update the docs to
remove that absolute path and show the project-standard invocation (`make
markdownlint`) (or a generic `make markdownlint MDLINT=...` if you want to
override) so the step is environment-agnostic—replace the line containing the
hard-coded `MDLINT=/root/.bun/bin/markdownlint-cli2` with the plain `make
markdownlint` invocation.
In `@docs/roadmap.md`:
- Around line 440-441: Update the roadmap checklist item that reads "9.7.4. Add
regression tests in `wireframe_testing`..." to avoid implying tests live under
`wireframe_testing`; change the phrase "in `wireframe_testing`" to "backed by
`wireframe_testing`" (or similar wording) so the item communicates that
regressions under the repository-root `tests/` directory are supported by the
`wireframe_testing` dev-dependency; locate the string "9.7.4. Add regression
tests in `wireframe_testing` for the `CodecError` taxonomy and recovery policy
behaviours defined in 9.1.2. Requires 9.1.2." and edit it accordingly.
In `@tests/test_codec_error_regressions.rs`:
- Around line 32-95: The test helpers extract_eof_error, classify_eof,
encoded_default_frame, truncated_payload_wire and the StrictRecoveryHook are
duplicated across tests; factor them into a shared test utility module (e.g.,
tests/utils/codec_helpers or a new tests::fixtures module) and update both
tests/test_codec_error_regressions.rs and
tests/fixtures/codec_error_regressions.rs to import the helpers from that shared
module; ensure you move any dependent helpers (e.g., new_test_codec,
encode_frame, LENGTH_HEADER_SIZE, write_network_u32) or re-export them as needed
and adjust visibility (pub(crate)) so functions like extract_eof_error,
classify_eof, encoded_default_frame, truncated_payload_wire and
StrictRecoveryHook compile without changes to their callers.
- Around line 257-273: The test currently only checks error Display fragments
from decode_frames_with_codec; change it to assert the concrete codec error by
invoking the decoder directly (use
wireframe::codec::examples::HotlineFrameCodec::new(...) and call its decode
method instead of the helper) and downcast the returned error to the codec's
error type (CodecError) then match the specific variant(s) (e.g.,
PayloadTooLarge / TruncatedHeader or the variant names used by your CodecError)
and assert the associated fields like max_frame_length or bytes remaining;
replace the string-contains assertion with these typed assertions so the test
fails if the error variant/policy changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd96ee71-a13e-42b0-83de-c33d8c74043b
📒 Files selected for processing (13)
docs/execplans/11-1-2-structured-logging-and-tracing-spans.mddocs/execplans/9-7-4-codec-error-taxonomy-regression-tests.mddocs/roadmap.mddocs/wireframe-testing-crate.mdsrc/codec.rstests/features/codec_error_regressions.featuretests/fixtures/codec_error_regressions.rstests/fixtures/mod.rstests/scenarios/codec_error_regressions_scenarios.rstests/scenarios/mod.rstests/steps/codec_error_regressions_steps.rstests/steps/mod.rstests/test_codec_error_regressions.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 38810
Overall comment 1 — deduplicate helpersNot resolved. Overall comment 2 —
|
…odec error regression tests - Introduce `tests/common/codec_error_regression_support.rs` for shared helper functions and types. - Refactor `tests/fixtures/codec_error_regressions.rs` and `tests/test_codec_error_regressions.rs` to use shared helpers. - Simplify EOF error classification and partial payload wire helpers. - Remove duplicated code and improve test reliability and maintainability. - Update roadmap and docs to reflect improved observability and testing. This restructuring enhances test clarity and reuse without changing test behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/test_codec_error_regressions.rs (1)
204-219:⚠️ Potential issue | 🟠 MajorAssert the structured codec taxonomy, not just
Displaytext.This test only matches
Displayfragments. If the decoder surfaces the wrongCodecErrorvariant or recovery policy whilst keeping a similar message, this regression suite still passes. Downcast the cause and assert the error type/policy.🤖 Prompt for AI Agents
In `tests/test_codec_error_regressions.rs`, update the `wireframe_testing_fixture_cases_continue_to_surface_expected_errors` test to assert the *structured* codec error, not merely the Display text. Steps: 1. Add two new fields to the `#[rstest]` case attributes alongside `wire`, `max_frame_length`, and `expected_fragment`: - `expected_variant: &'static str` — the value returned by `CodecError::error_type()` for each case (e.g. `"framing"`). - `expected_policy: RecoveryPolicy` — the value returned by `CodecError::default_recovery_policy()` for each case. 2. Update the two existing cases with the expected variant and policy values. 3. Inside the function body, after obtaining the `error` from `decode_frames_with_codec`, traverse the error chain to locate the underlying `CodecError` and assert its `error_type()` and `default_recovery_policy()`. 4. Keep the existing `Display`-fragment assertion so both structural and message-text regressions are covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codec_error_regressions.rs` around lines 204 - 219, Update the test wireframe_testing_fixture_cases_continue_to_surface_expected_errors to assert the structured CodecError and recovery policy: extend each #[case] in the rstest to include expected_variant: &'static str (the string returned by CodecError::error_type()) and expected_policy: wireframe::codec::CodecError::RecoveryPolicy (or import RecoveryPolicy) alongside wire, max_frame_length, expected_fragment; after calling decode_frames_with_codec and getting error, traverse the error chain (e.g., using error.chain() or downcast_ref) to find the underlying CodecError, then assert that codec_error.error_type() == expected_variant and codec_error.default_recovery_policy() == expected_policy, while keeping the existing Display-fragment assertion; reference symbols: wireframe_testing_fixture_cases_continue_to_surface_expected_errors, decode_frames_with_codec, CodecError, error_type(), default_recovery_policy(), RecoveryPolicy.tests/fixtures/codec_error_regressions.rs (1)
14-14: 🛠️ Refactor suggestion | 🟠 MajorNarrow
pubtopub(crate)for test-internal APIs.The
TestResultre-export and subsequent public items (CodecErrorRegressionsWorld, fixture function, public methods) are only consumed within the same integration-test binary. Restrict visibility topub(crate)to avoid implying external consumption.🤖 Prompt for AI Agents
In `tests/fixtures/codec_error_regressions.rs`, narrow every unnecessarily broad `pub` declaration to `pub(crate)`. All call sites are within the same integration-test binary. Specifically: 1. Line 14: `pub use wireframe_testing::TestResult;` → `pub(crate) use wireframe_testing::TestResult;` 2. Line 33: `pub struct CodecErrorRegressionsWorld` → `pub(crate) struct CodecErrorRegressionsWorld` 3. Line 51: `pub fn codec_error_regressions_world()` → `pub(crate) fn codec_error_regressions_world()` 4. All `pub fn` methods on `impl CodecErrorRegressionsWorld` (lines 56, 64, 72, 80, 87, 94, 104, 119, 152, 156): → `pub(crate) fn` Do not add `#[allow]` attributes. Verify compilation after changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/codec_error_regressions.rs` at line 14, Change all externally-visible test items to crate-only visibility: replace the top-level reexport `pub use wireframe_testing::TestResult;` with `pub(crate)`, change the type declaration `pub struct CodecErrorRegressionsWorld` to `pub(crate)`, change the fixture constructor `pub fn codec_error_regressions_world()` to `pub(crate)`, and change every `pub fn` on `impl CodecErrorRegressionsWorld` (the methods defined at the noted lines) to `pub(crate) fn`; after edits run a build to ensure no other call sites require public visibility.
🤖 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_codec_error_regressions.rs`:
- Around line 119-127: The test taxonomy_cases_emit_expected_recovery_labels
only includes a CleanClose EOF case; add two new TaxonomyCase entries for
CodecError::Eof(EofError::MidHeader) and CodecError::Eof(EofError::MidFrame) so
the test covers all EofError variants. For each new case set expected_type:
"eof", expected_policy: RecoveryPolicy::Disconnect, expected_disconnect: true,
and expected_clean_close: false, and insert them alongside the existing
TaxonomyCase::clean_close entry so the test asserts error_type(),
default_recovery_policy(), should_disconnect(), and is_clean_close() for
MidHeader and MidFrame independently.
- Around line 148-163: Add a new ZeroLengthPayload EOF test case: update the
rstest cases to include
#[case::zero_length_payload(DefaultEofCase::ZeroLengthPayload,
EofError::CleanClose)] (or the expected EofError variant used for clean frame
consumption), add a ZeroLengthPayload variant to the DefaultEofCase enum,
implement its wire() arm to return a 4-byte zero header (vec![0,0,0,0]) and
ensure consumes_complete_frame() returns true for ZeroLengthPayload (i.e.,
matches!(self, Self::CleanClose | Self::ZeroLengthPayload)); this will exercise
the header-only frame followed immediately by EOF in
tests/test_codec_error_regressions.rs.
---
Duplicate comments:
In `@tests/fixtures/codec_error_regressions.rs`:
- Line 14: Change all externally-visible test items to crate-only visibility:
replace the top-level reexport `pub use wireframe_testing::TestResult;` with
`pub(crate)`, change the type declaration `pub struct
CodecErrorRegressionsWorld` to `pub(crate)`, change the fixture constructor `pub
fn codec_error_regressions_world()` to `pub(crate)`, and change every `pub fn`
on `impl CodecErrorRegressionsWorld` (the methods defined at the noted lines) to
`pub(crate) fn`; after edits run a build to ensure no other call sites require
public visibility.
In `@tests/test_codec_error_regressions.rs`:
- Around line 204-219: Update the test
wireframe_testing_fixture_cases_continue_to_surface_expected_errors to assert
the structured CodecError and recovery policy: extend each #[case] in the rstest
to include expected_variant: &'static str (the string returned by
CodecError::error_type()) and expected_policy:
wireframe::codec::CodecError::RecoveryPolicy (or import RecoveryPolicy)
alongside wire, max_frame_length, expected_fragment; after calling
decode_frames_with_codec and getting error, traverse the error chain (e.g.,
using error.chain() or downcast_ref) to find the underlying CodecError, then
assert that codec_error.error_type() == expected_variant and
codec_error.default_recovery_policy() == expected_policy, while keeping the
existing Display-fragment assertion; reference symbols:
wireframe_testing_fixture_cases_continue_to_surface_expected_errors,
decode_frames_with_codec, CodecError, error_type(), default_recovery_policy(),
RecoveryPolicy.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 88e76211-88de-4a8b-8b85-d27a57ae8395
📒 Files selected for processing (5)
docs/execplans/9-7-4-codec-error-taxonomy-regression-tests.mddocs/roadmap.mdtests/common/codec_error_regression_support.rstests/fixtures/codec_error_regressions.rstests/test_codec_error_regressions.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22895
✅ Overall comment 1 — deduplicate helpers — RESOLVED
✅ Overall comment 2 —
|
…estrict test code visibility - Introduced a new DefaultEofCase variant `ZeroLengthPayload` representing an EOF with zero length payload. - Added tests verifying the zero-length payload case is treated as a clean close. - Updated exhaustive match branches and test cases to include this new EOF case. - Restricted visibility of the `CodecErrorRegressionsWorld` struct and its methods from public to `pub(crate)` to encapsulate test fixtures and helpers. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_codec_error_regressions.rs (1)
233-249:⚠️ Potential issue | 🟠 MajorPreserve the concrete
CodecErroron this decode path.Stop asserting only on the
Displayfragment.src/wireframe_testing/src/helpers/codec_ext.rscurrently wraps decoder failures withio::Error::new(..., format!(...)), so the concreteCodecErroris gone before this test runs; the regression still passes if the decoder emits the wrong variant or recovery policy with similar text. Preserve the source error in that helper, or add a companion helper that returns the concrete error, then asserterror_type()anddefault_recovery_policy()here as well.Verify the helper first. Expected result: if it still uses
format!(...)insideio::Error::new, downcasting from this test is impossible.#!/bin/bash set -euo pipefail helper_file="$(fd 'codec_ext\.rs$' . | head -n1)" test_file="$(fd 'test_codec_error_regressions\.rs$' . | head -n1)" test -n "$helper_file" test -n "$test_file" echo "== $helper_file ==" rg -n -C2 'decode_frames_with_codec|io::Error::new|codec decode failed|codec decode_eof failed' "$helper_file" echo echo "== $test_file ==" sed -n '233,249p' "$test_file"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bdcca56-97c7-4d20-9913-d29d154c41c1
📒 Files selected for processing (2)
tests/fixtures/codec_error_regressions.rstests/test_codec_error_regressions.rs
This change wraps CodecError variants inside io::Error without losing the original error type, allowing it to be recovered later via error source chaining. Tests were updated to traverse error sources for CodecError extraction. Added new regression tests to verify error preservation in decode failures. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Refactored tests in test_codec_error_regressions.rs to replace raw wire byte vectors and max_frame_length parameters with the DefaultEofCase enum to represent different EOF scenarios. Updated test cases to use this enum for improved clarity and maintainability. Also standardized codec creation with a fixed max frame length in the tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…st case Shortened the expected_fragment string in the test to "premature EOF" instead of the longer message. This refinement keeps the test focused and less brittle to minor message changes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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.
Summary
wireframe_testing, including unit tests, BDD-style scenarios, and a dedicated regression world.Changes
Rationale
wireframe_testing.Plan of work (high level)
Stage A: create failing regression coverage first
tests/test_codec_error_regressions.rsfocused on 9.1.2 semantics.Stage B: add only the minimum missing test-support API
wireframe_testingwith minimal helpers to support new regressions (kept narrowly scoped).Stage C: add behavioural coverage through
rstest-bddtests/features/codec_error_regressions.featureand corresponding fixtures/scenarios/steps.Stage D: update documentation and roadmap
docs/wireframe-testing-crate.mdanddocs/roadmap.mdto reflect new testing approach and 9.7.4 completion.Stage E: validate red, green, and full gates
Testing plan
Documentation impact
docs/wireframe-testing-crate.mdto reflect regression testing patterns and observability assertions.docs/roadmap.mdto reflect 9.7.4 status as completed once gates are green.Approval notes
◳ Generated by DevBoxer ◰
Summary by Sourcery
Add codec EOF error classification context and a regression suite validating codec error taxonomy, recovery policy labels, and observability labels via wireframe_testing, and mark the related roadmap item complete.
New Features:
Bug Fixes:
Enhancements:
📎 Task: https://www.devboxer.com/task/9ccbe999-9e5b-4d0e-9fc4-19617abe61d6