OpenZeppelin / OpenZeppelin/openzeppelin-relayer
system_cleanup queue-metadata cleanup is not unit-testable without Redis (add Redis to CI or refactor to be generic)
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 150
- Forks
- 72
- Avg merge
- 8d 9h
- Merged PRs (30d)
- 2
Description
Summary
Follow-up to #816 (fix for #815). The system_cleanup handler that prunes Redis
queue metadata currently has ~0% patch coverage on its core branch, and there is
no straightforward way to unit-test it today. This issue tracks making that code
path testable.
Background
#816 changed handle_cleanup_request in
src/jobs/handlers/system_cleanup_handler.rs to source the Redis pool from the
queue backend instead of the transaction repository, so cleanup runs whenever
the queue backend is Redis regardless of REPOSITORY_STORAGE_TYPE. Codecov
flagged the changed lines as uncovered.
Why it's hard to cover right now
All of the changed lines sit behind the backend_type() == Redis guard, and the
only way to obtain a non-None redis_connections() is through Queue::setup(),
which opens live Redis ConnectionManagers. Consequently:
- A plain unit test cannot reach those lines without a running Redis.
- CI has no Redis service in the coverage job (
.github/workflows/ci.yaml), and
the existing*_redis.rstests are all#[ignore]d, so a Redis-gated
integration test would not run in CI (and thus wouldn't contribute coverage). handle_cleanup_requesttakes a concreteThinData<DefaultAppState>(hard-wired
to the concreteJobProducer), so aMockJobProducerTraitcannot be injected
as-is.
Proposed options (any one, or a combination)
-
Make the inner handler generic over the job producer, mirroring the existing
relayer_health_check_handlerpattern (public concrete wrapper +
generic_impl). This allows injectingMockJobProducerTraitand unit-testing
the two skip branches (non-Redis backend; Redis backend with no connections)
in normal CI — covers most of the diff without any infra. -
Add a Redis service to the coverage CI job and either un-
#[ignore]a
focused integration test or add a new one that exercises the full
prune path (<queue>:{done,failed,dead}+:data/:data::result). -
Add a Redis-gated
#[ignore]d integration test (mirrors the current
*_redis.rsconvention) that documents/verifies the end-to-end behavior even
if it doesn't count toward CI coverage.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with src/jobs/handlers/system_cleanup_handler.rs and read handle_cleanup_request alongside the relayer_health_check_handler pattern. Then inspect .github/workflows/ci.yaml and the existing *_redis.rs tests to choose between generic unit coverage and Redis-backed integration coverage. Done means the Redis cleanup branches have meaningful test coverage in the intended CI path, with the queue metadata keys exercised where applicable.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, redis, rust
- Domain
- backend, ci-cd, testing-qa
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100