stacklok / stacklok/toolhive

Add condition transition tests for 3 VirtualMCPServer conditions

Open
#4,625 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement go kubernetes operator vmcp
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Summary

Three VirtualMCPServer status conditions (BackendsDiscovered, EmbeddingServerReady, AuthServerConfigValidated) have no tests for their transition lifecycle. Other VirtualMCPServer conditions (Ready, ConfigRef, ScalingReady, etc.) have well-tested transition paths, but these three are set during reconciliation without any verification of their transitions.

Severity: SHOULD FIX
Area: Testing
Breaking: No

Location

  • VirtualMCPServer controller tests

Problem

The following conditions are set during reconciliation but their transitions (True -> False, False -> True, initial setting) are not verified by any test:

  1. BackendsDiscovered — set when backend MCP servers are discovered and validated
  2. EmbeddingServerReady — set when an associated EmbeddingServer reaches ready state
  3. AuthServerConfigValidated — set when the auth server configuration passes validation

Impact

  • Regressions in condition-setting logic for these three conditions would go undetected
  • The "condition persisted on recovery from transient errors" pattern (as implemented in recent commit 69ea2de7) is not tested for these conditions
  • Status reporting for backend discovery, embedding readiness, and auth validation is not covered

Recommended Fix

Add condition lifecycle tests following the established patterns:

Describe("BackendsDiscovered condition", func() {
    It("should set BackendsDiscovered=True when backends are found", func() {
        // Setup VirtualMCPServer with valid backend refs
        // Reconcile
        // Verify condition is True with correct reason/message
    })

    It("should set BackendsDiscovered=False when backends are missing", func() {
        // Setup VirtualMCPServer with invalid backend refs
        // Reconcile
        // Verify condition is False with correct reason/message
    })

    It("should persist condition on recovery from transient error", func() {
        // Trigger transient error, then recover
        // Verify condition transitions correctly
    })
})

Repeat the pattern for EmbeddingServerReady and AuthServerConfigValidated.

Related

  • Commit 69ea2de7 (condition persistence pattern) — the pattern these tests should follow

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 in the VirtualMCPServer controller tests and compare existing Ready, ConfigRef, and ScalingReady transition coverage with the condition persistence pattern from commit 69ea2de7. Add lifecycle coverage for BackendsDiscovered, EmbeddingServerReady, and AuthServerConfigValidated, including initial, True/False transitions, recovery, and reason/message assertions; run the controller test suite to verify the cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.