Skip to content

test(falkordb): skip when the port answers but is not FalkorDB - #3132

Closed
guitelesc wants to merge 1 commit into
Graphify-Labs:v8from
guitelesc:fix/falkordb-guard-checks-service-not-liveness
Closed

test(falkordb): skip when the port answers but is not FalkorDB#3132
guitelesc wants to merge 1 commit into
Graphify-Labs:v8from
guitelesc:fix/falkordb-guard-checks-service-not-liveness

Conversation

@guitelesc

Copy link
Copy Markdown
Contributor

_connect() guards on ping(), which only proves something answers on 6379. A plain Redis — or an SSH tunnel forwarding one, which is what I hit — replies to PING but has no graph module, so the guard passes and both tests fail on unknown command 'GRAPH.QUERY' instead of skipping.

The module docstring says these are a no-op outside CI. That holds only because CI has nothing on 6379: the guard was validated against the empty case, never against a wrong occupant. A dev box with a Redis already on that port is exactly where it breaks.

The fix

Probe GRAPH.LIST after the ping, so the guard identifies the service rather than mere liveness. It is read-only, creates no keys, and anything that is not FalkorDB/RedisGraph rejects it as an unknown command:

probe FalkorDB plain Redis
PING ✅ True ✅ True — the bug
GRAPH.LIST [] unknown command
MODULE LIST ✅ has graph [] — succeeds on both, needs content parsing

The two failure modes now skip with distinct messages (unreachable vs. wrong service).

Testing

Verified against all three environments, including the one that matters most — a guard that always skips would "fix" the failure while silently disabling the tests forever:

plain Redis on the port   -> 2 skipped  (was 2 failed)
falkordb/falkordb:latest  -> 2 passed   (guard does not over-skip)
nothing listening         -> 2 skipped

uv run pytest tests/ -q — 5086 passed, 11 skipped, 0 failed.

No automated regression test: reproducing this needs a wrong-service-on-the-port, i.e. the external service the default CI deliberately avoids. Happy to add one behind a marker if you'd prefer it.

🤖 Generated with Claude Code

`_connect()` guarded on `ping()`, which only proves *something* answers on
6379. A plain Redis -- or an SSH tunnel forwarding one, common on a dev box --
replies to PING but has no graph module, so the guard passed and both tests
FAILED on `unknown command 'GRAPH.QUERY'` instead of skipping.

The docstring's claim that these are a no-op outside CI held only because CI
has nothing on 6379: the guard was validated against the empty case, never
against a wrong occupant.

Probe `GRAPH.LIST` after the ping so the guard identifies the service rather
than mere liveness. It is read-only, creates no keys, and anything that is not
FalkorDB/RedisGraph rejects it as an unknown command. The two failure modes
now skip with distinct messages (unreachable vs. wrong service).

Verified against all three environments:
  plain Redis on the port -> 2 skipped (was 2 failed)
  falkordb/falkordb:latest -> 2 passed  (guard does not over-skip)
  nothing listening        -> 2 skipped

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).


Graphify review — findings

Tightens the FalkorDB integration test's connection guard to skip—rather than fail—when a non-FalkorDB server answers on the port. _connect now follows its ping() liveness check with a read-only GRAPH.LIST probe, so a plain Redis (or an SSH tunnel forwarding one) that lacks the graph module is detected and skipped instead of erroring out on unknown command 'GRAPH.QUERY'.

No blocking issues surfaced.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 8 functions depend on the 8 functions this change touches.

Health — grade A; no new coupling hotspots.

Verification — 8 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 8 function(s) in the blast radius were not formally verified this run

@safishamsi

Copy link
Copy Markdown
Collaborator

Shipped in v0.9.51 via authorship-preserving cherry-pick so you keep contributor-graph credit. Thanks @guitelesc! Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.51

@safishamsi safishamsi closed this Aug 28, 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.

2 participants