Skip to content

Fix test failures after jettison library update#61

Merged
echarrod merged 4 commits intorenovate/github.com-luno-jettison-digestfrom
copilot/update-tests-after-jettison-lib-change
Feb 4, 2026
Merged

Fix test failures after jettison library update#61
echarrod merged 4 commits intorenovate/github.com-luno-jettison-digestfrom
copilot/update-tests-after-jettison-lib-change

Conversation

Copy link
Contributor

Copilot AI commented Oct 20, 2025

  • Add error codes to sentinel errors filterErr and deserializationErr so they can be matched with errors.Is() after wrapping
  • Add error code to metadataEventFilterErr for consistency
  • Update wrapping functions to include error codes
  • Update test expectations to use sentinel errors with codes
  • Fix batch test to use proper sentinel error
  • Verify tests pass after changes
  • Run codeql_checker to ensure no security vulnerabilities
  • Merge with latest main branch via renovate/github.com-luno-jettison-digest

Summary

Fixed test failures after jettison library update and merged with latest main.

Changes Made:

  1. Added error codes to sentinel errors for proper errors.Is() matching:

    • filterErr in errors.go - code ERR_d5e8f7a9b2c3d4e5
    • deserializationErr in filters/metadata.go - code ERR_a1b2c3d4e5f6a7b8
    • metadataEventFilterErr in filters/metadata.go - code ERR_c1f2e3d4a5b6c7d8
    • ErrInvalidBatchConfig in rpatterns/batch.go - code ERR_invalid_batch_config
  2. Updated error wrapping functions to include error codes:

    • asFilterErr() in errors.go
    • asDeserializationErr() and makeMetadataEventFilterErr() in filters/metadata.go
  3. Updated test expectations:

    • Modified test case for "constructed deserialization error" to expect false
    • Added sentinel test errors with codes in metadata_test.go
    • Updated batch tests to use sentinel errors
  4. Merged with latest:

    • Merged renovate/github.com-luno-jettison-digest branch into this branch
    • This includes all latest changes from main plus the newer jettison update (v0.0.0-20260204095351-d930f4b24a4f)
    • Resolved conflicts by taking the newer dependency versions

All tests pass with no security vulnerabilities.

Original prompt

This section details on the original issue you should resolve

<issue_title>Update tests after jettison lib change</issue_title>
<issue_description>#41 has introduced some test failured after the jettison library change

The failure is caused by the dead letter consumer not being able to record an error, which leads to the original error being returned and the test failing. This is visible in the logs:

dead letter consumer cannot record an error: consumer error
at rpatterns/deadletter.go:65
at rpatterns/deadletter_test.go:139

The relevant code in rpatterns/deadletter.go shows that when the error cannot be recorded (i.e., the inserter returns an error), the consumer returns the original error instead of swallowing it:

64| } else if iErr := d.inserter(ctx, ev.ID, d.name, err.Error()); iErr != nil {
65|     log.Error(ctx, errors.Wrap(err, "dead letter consumer cannot record an error"),
66|         j.MKS{"consumer": d.name, "eventID": ev.ID, "errMsg": err.Error(), "recErrMsg": iErr.Error()})
67|     return err
68| }

The associated test case in rpatterns/deadletter_test.go expects this behaviour:

72|     expErr:         []error{consumerErr, consumerErr, consumerErr},

However, the logs indicate that this error scenario is occurring repeatedly, which suggests either:

  • The inserter function used in tests (or in production) is returning an error (deadLetterErr) when it should not, or
  • The test is set up to simulate a recording failure, but the code is not handling this as expected.

Solution:

  1. Ensure that the error inserter function (the eFn passed to NewDeadLetterConsumer) does not return an error unless we are explicitly testing the failure path.
  2. If we are testing the failure path, verify that the test expects the consumer to return the original error, as in the "0 retries 1 error failed to write" or similar test cases.

If we want to make the consumer ignore inserter errors and continue (not recommended unless we can safely drop error records), we could change:

67|     return err

to:

67|     return nil

But be aware this would change the intended semantics and could hide real operational issues.

Recommended action:
Check the implementation of the error inserter. If it is intentionally returning errors, adjust the test expectations. If not, ensure it behaves as expected in normal operation.

For reference, here is the relevant file: rpatterns/deadletter.go@e240053.
</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #60


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Summary by CodeRabbit

  • New Features

    • Added a new error type to distinguish invalid batch configuration scenarios, improving error handling clarity.
  • Chores

    • Updated dependencies including gRPC to v1.75.1 and enhanced error context tracking across internal error handling paths.

@coderabbitai
Copy link

coderabbitai bot commented Oct 20, 2025

Walkthrough

The pull request introduces error context codes across the codebase using the jettison library's context parameter (j.C()). Changes span errors.go, filters/metadata.go, and rpatterns/batch.go, adding structured error context identifiers to error construction and wrapping calls. A new public error variable ErrInvalidBatchConfig is introduced in rpatterns/batch.go, and corresponding test files are updated to use the new error constants and validate against the refactored error-handling paths. The go.mod file reflects updated dependencies, including a jettison version bump.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes exhibit moderate complexity with mixed patterns: repetitive error context code additions (j.C() wrapping) applied across multiple files with slight variations, a new public error variable requiring API validation, and test updates that need verification for correctness. The heterogeneous nature—spanning error construction, public exports, and test expectations—requires separate reasoning for each modified area. Dependency version changes add minor verification overhead.

Poem

🐰 Hop along, dear errors now wear their bright codes,
Context codes gleaming on reflex's roads,
From metadata filters to batches so fine,
Each error now labelled—ERR\_codes align!
Tests bounce with glee, all expectations renewed,
The jettison library's path is construed!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Linked Issues Check ❓ Inconclusive The PR successfully addresses the primary coding requirement from linked issue #60 by adding error codes to sentinel errors across multiple modules and updating error-wrapping functions to include these codes. Changes to errors.go (ERR_d5e8f7a9b2c3d4e5), filters/metadata.go (ERR_a1b2c3d4e5f6a7b8 and ERR_c1f2e3d4a5b6c7d8), and rpatterns/batch.go (ERR_invalid_batch_config) with corresponding test updates align with the objective to ensure sentinel errors are matched correctly under the new jettison behaviour. However, the provided summary does not show visible changes to rpatterns/deadletter.go or deadletter_test.go, which are specifically mentioned in issue #60 as the problematic area, leaving it unclear whether the "ensure dead letter consumer's inserter behaviour aligns with tests" objective is fully addressed. To verify full compliance, please confirm that the deadletter consumer test failures mentioned in issue #60 are resolved either through the sentinel error code changes or through separate changes to deadletter.go and deadletter_test.go that may not be included in the provided summary. If deadletter-specific changes are necessary but absent, they should be added to this PR or addressed in a follow-up.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Fix test failures after jettison library update" clearly and directly relates to the main change in the pull request. The PR adds error codes to sentinel errors across multiple modules (errors.go, filters/metadata.go, rpatterns/batch.go) and updates error-wrapping functions and tests to use these codes, which collectively addresses the test failure issue caused by the jettison library update. The title is concise, specific, and accurately conveys the primary context without unnecessary noise.
Out of Scope Changes Check ✅ Passed All visible code changes are directly related to addressing test failures caused by the jettison library update. The modifications consistently focus on adding error codes to sentinel errors (errors.go, filters/metadata.go, rpatterns/batch.go), updating error-wrapping logic to include these codes, and adjusting test expectations accordingly (filters/metadata_test.go, rpatterns/batch_test.go). The go.mod dependency update is necessary and appropriate for supporting the jettison library changes. No extraneous or unrelated changes are evident in the provided summary.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch copilot/update-tests-after-jettison-lib-change

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a4b652 and 9dd0d9f.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • errors.go (2 hunks)
  • filters/metadata.go (3 hunks)
  • filters/metadata_test.go (4 hunks)
  • go.mod (3 hunks)
  • rpatterns/batch.go (2 hunks)
  • rpatterns/batch_test.go (2 hunks)
🔇 Additional comments (13)
rpatterns/batch_test.go (2)

173-173: LGTM! Correct use of the exported error constant.

The test now properly validates against the public ErrInvalidBatchConfig sentinel error, aligning with the updated error-matching approach.


91-91: No action required—errEvents is properly defined.

The variable errEvents is defined in rpatterns/bootstrap_test.go at line 20 and is accessible across all test files within the rpatterns package. The references at lines 91, 148, and 154 in batch_test.go are correct.

filters/metadata.go (3)

21-22: LGTM! Error codes added for reliable matching.

The sentinel errors now include context codes, enabling the updated jettison library to match errors by code rather than message.


48-48: LGTM! Consistent error code in wrapping function.

The error code matches the sentinel error declaration, ensuring wrapped errors are correctly identified by IsDeserializationErr.


57-58: LGTM! Error construction maintains consistency.

Prepending the error code to the options ensures all constructed errors include the sentinel's code for reliable matching.

errors.go (2)

29-29: LGTM! Error code added for reliable matching.

The filterErr sentinel now includes a context code, consistent with the error-handling updates across the codebase.


63-63: LGTM! Consistent error code in wrapping function.

The error code matches the filterErr sentinel, ensuring IsFilterErr correctly identifies wrapped errors.

rpatterns/batch.go (2)

25-28: LGTM! New public error for configuration validation.

The exported ErrInvalidBatchConfig sentinel allows callers to reliably detect configuration errors, consistent with the error-handling improvements in this PR.


101-101: LGTM! Correct use of the exported error constant.

Returning the sentinel error instead of an inline error enables reliable error matching in tests and calling code.

filters/metadata_test.go (3)

7-7: LGTM! Test error constants with codes defined.

The test-specific error constants with codes enable reliable error matching in test assertions, consistent with the updated jettison behaviour.

Also applies to: 14-17


83-83: LGTM! Consistent use of test error constants.

The tests now use the predefined error constants with codes, ensuring reliable error matching under the new jettison behaviour.

Also applies to: 88-88, 97-97, 99-99


168-168: LGTM! Test expectation correctly updated for code-based matching.

The change from true to false reflects the updated jettison behaviour: an error constructed with only the message (no code) no longer matches a sentinel error that has a code. This is the core fix described in the PR objectives.

go.mod (1)

8-8: All updated dependencies have been verified as secure. No security vulnerabilities were detected for:

  • github.com/luno/jettison (pseudo-version v0.0.0-20251004142841-a8f926feeafb)
  • google.golang.org/grpc v1.75.1 (well beyond all known vulnerable ranges)
  • Colour-related transitive dependencies (fatih/color, mattn/go-colorable, mattn/go-isatty)

The dependency versions are compatible and secure for use.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Add error codes to sentinel errors for proper matching with errors.Is().
Update test expectations to use sentinel errors with codes instead of
creating new error instances inline.

Co-authored-by: dasco144 <10575019+dasco144@users.noreply.github.com>
Copilot AI changed the title [WIP] Update tests after jettison lib change Fix test failures after jettison library update Oct 20, 2025
Copilot AI requested a review from dasco144 October 20, 2025 15:39
@renovate renovate bot force-pushed the renovate/github.com-luno-jettison-digest branch from 4371be5 to 9a4b652 Compare October 20, 2025 16:29
@dasco144
Copy link
Contributor

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Oct 20, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@renovate renovate bot force-pushed the renovate/github.com-luno-jettison-digest branch 2 times, most recently from a95ac81 to 56faa88 Compare November 13, 2025 20:36
@renovate renovate bot force-pushed the renovate/github.com-luno-jettison-digest branch 2 times, most recently from 9ad917d to d7231e5 Compare December 20, 2025 17:52
@renovate renovate bot force-pushed the renovate/github.com-luno-jettison-digest branch 3 times, most recently from bfd66ce to 82331fa Compare February 4, 2026 12:53
@echarrod
Copy link
Contributor

echarrod commented Feb 4, 2026

@copilot can you rebase and fix the merge conflicts from main

@echarrod echarrod marked this pull request as ready for review February 4, 2026 15:12
Copy link
Contributor Author

Copilot AI commented Feb 4, 2026

@copilot can you rebase and fix the merge conflicts from main

I've successfully rebased the branch onto renovate/github.com-luno-jettison-digest (which is based on main) and resolved all conflicts. The rebase:

  • Removed the old jettison update commit (4371be5) since renovate/github.com-luno-jettison-digest already has a newer jettison update (82331fa)
  • Kept the two fix commits (Initial plan + Fix test failures)
  • All tests pass with the newer jettison version

However, I'm unable to force-push the rebased history using the available tools due to a technical limitation. The local branch is ready with commits:

  • 3290e0c: Trigger push after rebase
  • 9b806f9: Fix test failures after jettison library update
  • fbe654d: Initial plan
  • 82331fa: renovate: Update github.com/luno/jettison digest to d930f4b (base)

The branch needs to be force-pushed to update the remote. Could you either:

  1. Grant force-push permissions to the automation, or
  2. Manually force-push the local copilot/update-tests-after-jettison-lib-change branch that's been rebased

@echarrod
Copy link
Contributor

echarrod commented Feb 4, 2026

@copilot can you merge main into this branch then so you don't have to force push

Copy link
Contributor Author

Copilot AI commented Feb 4, 2026

@copilot can you merge main into this branch then so you don't have to force push

Done! I've merged renovate/github.com-luno-jettison-digest (which includes latest main + newer jettison update) into this branch. The merge resolved all conflicts by taking the newer dependency versions. All tests pass successfully. The changes are now pushed in commit 4ebd11c.

@echarrod echarrod merged commit f0fe56b into renovate/github.com-luno-jettison-digest Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants