OpenZeppelin / OpenZeppelin/openzeppelin-relayer

system_cleanup queue-metadata cleanup is not unit-testable without Redis (add Redis to CI or refactor to be generic)

Open
#830 0 comments 0 reactions 0 assignees View on GitHub

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.rs tests are all #[ignore]d, so a Redis-gated
    integration test would not run in CI (and thus wouldn't contribute coverage).
  • handle_cleanup_request takes a concrete ThinData<DefaultAppState> (hard-wired
    to the concrete JobProducer), so a MockJobProducerTrait cannot be injected
    as-is.

Proposed options (any one, or a combination)

  1. Make the inner handler generic over the job producer, mirroring the existing
    relayer_health_check_handler pattern (public concrete wrapper +
    generic _impl). This allows injecting MockJobProducerTrait and 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.

  2. 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).

  3. Add a Redis-gated #[ignore]d integration test (mirrors the current
    *_redis.rs convention) that documents/verifies the end-to-end behavior even
    if it doesn't count toward CI coverage.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.