Skip to content

refactor: replace magic numbers with named constants#980

Open
Priyanshu-u07 wants to merge 1 commit into
Kuadrant:mainfrom
Priyanshu-u07:add-magic-number-constants
Open

refactor: replace magic numbers with named constants#980
Priyanshu-u07 wants to merge 1 commit into
Kuadrant:mainfrom
Priyanshu-u07:add-magic-number-constants

Conversation

@Priyanshu-u07
Copy link
Copy Markdown
Contributor

@Priyanshu-u07 Priyanshu-u07 commented May 14, 2026

Description

Replaces all hardcoded magic numbers across the infrastructure modules with named constants from a single centralized file.

Part of #930

Changes

New file

  • testsuite/utils/constants.py: Central module with multiples named constants organized by domain (Kubernetes, Policy, Prometheus, SpiceDB, Envoy, etc.)

File Rename

  • testsuite/utils.py to testsuite/utils/__init__.py: Converted to a package to support the new constants.py submodule. No code changes and all existing imports unaffected.

Fix

  • Removed duplicate validator in config/__init__.py: There were two DefaultValueValidator("mockserver.url", ...) entries.

Refactored Files(23)

Files
  • testsuite/backend/__init__.py
  • testsuite/backend/httpbin.py
  • testsuite/backend/llm_sim.py
  • testsuite/backend/mockserver.py
  • testsuite/config/__init__.py
  • testsuite/gateway/envoy/__init__.py
  • testsuite/gateway/gateway_api/gateway.py
  • testsuite/gateway/gateway_api/grpc_route.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/grpc/__init__.py
  • testsuite/httpx/__init__.py
  • testsuite/kuadrant/extensions/oidc_policy.py
  • testsuite/kuadrant/policy/__init__.py
  • testsuite/kuadrant/policy/dns.py
  • testsuite/kuadrant/policy/rate_limit.py
  • testsuite/kuadrant/policy/tls.py
  • testsuite/kubernetes/__init__.py
  • testsuite/kubernetes/deployment.py
  • testsuite/kubernetes/service.py
  • testsuite/prometheus.py
  • testsuite/spicedb/spicedb.py
  • testsuite/tracing/jaeger.py
  • testsuite/tracing/tempo.py

Verification

  • make reformat: Passed
  • make commit-acceptance: Passed
  • All constant values match original hardcoded values exactly. No behavioral change

Summary by CodeRabbit

  • Chores

    • Centralised timing, retry and port configuration into a shared constants module for consistent behaviour.
    • Updated defaults to use shared timeouts across Kubernetes operations, service readiness, deployments, policies, tracing and observability flows.
    • Standardised port definitions for HTTP API, mock services and inter-service communication.
  • Documentation

    • Added contribution guideline to use the centralised constants for timeouts, retries and ports.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7ab0f643-705a-46b7-a231-fc9bcd8bb895

📥 Commits

Reviewing files that changed from the base of the PR and between ceddf01 and 73bb0ac.

📒 Files selected for processing (26)
  • CONTRIBUTING.md
  • testsuite/backend/__init__.py
  • testsuite/backend/httpbin.py
  • testsuite/backend/llm_sim.py
  • testsuite/backend/mockserver.py
  • testsuite/config/__init__.py
  • testsuite/gateway/envoy/__init__.py
  • testsuite/gateway/gateway_api/gateway.py
  • testsuite/gateway/gateway_api/grpc_route.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/grpc/__init__.py
  • testsuite/httpx/__init__.py
  • testsuite/kuadrant/extensions/oidc_policy.py
  • testsuite/kuadrant/policy/__init__.py
  • testsuite/kuadrant/policy/dns.py
  • testsuite/kuadrant/policy/rate_limit.py
  • testsuite/kuadrant/policy/tls.py
  • testsuite/kubernetes/__init__.py
  • testsuite/kubernetes/deployment.py
  • testsuite/kubernetes/service.py
  • testsuite/prometheus.py
  • testsuite/spicedb/spicedb.py
  • testsuite/tracing/jaeger.py
  • testsuite/tracing/tempo.py
  • testsuite/utils/__init__.py
  • testsuite/utils/constants.py
✅ Files skipped from review due to trivial changes (3)
  • CONTRIBUTING.md
  • testsuite/tracing/tempo.py
  • testsuite/backend/init.py
🚧 Files skipped from review as they are similar to previous changes (20)
  • testsuite/kuadrant/policy/dns.py
  • testsuite/grpc/init.py
  • testsuite/kubernetes/deployment.py
  • testsuite/gateway/gateway_api/gateway.py
  • testsuite/kuadrant/policy/tls.py
  • testsuite/kuadrant/policy/rate_limit.py
  • testsuite/tracing/jaeger.py
  • testsuite/gateway/gateway_api/grpc_route.py
  • testsuite/config/init.py
  • testsuite/backend/httpbin.py
  • testsuite/backend/mockserver.py
  • testsuite/kubernetes/init.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/utils/constants.py
  • testsuite/httpx/init.py
  • testsuite/gateway/envoy/init.py
  • testsuite/prometheus.py
  • testsuite/backend/llm_sim.py
  • testsuite/kuadrant/policy/init.py
  • testsuite/kubernetes/service.py

📝 Walkthrough

Walkthrough

This pull request adds a centralized constants module testsuite/utils/constants.py and updates many test-suite modules to import and use those shared timeout, port, and retry constants instead of hardcoded literals.

Changes

Configuration Constants Centralisation

Layer / File(s) Summary
Centralised constants module definition
testsuite/utils/constants.py
New module defines Kubernetes wait/delete/readiness timeouts, policy enforcement timeouts and waits, Prometheus/tracing/HTTP retry and interval constants, SpiceDB client timeouts/retry/ports, service/container port numbers, gRPC call timeout, Envoy workaround timings, and OIDC post-enforcement wait.
Kubernetes core timeouts
testsuite/kubernetes/__init__.py
KubernetesObject.delete() uses K8S_DELETE_TIMEOUT and wait_until() defaults timelimit to K8S_WAIT_UNTIL_TIMEOUT.
Kubernetes Deployment and Service readiness/deletion
testsuite/kubernetes/deployment.py, testsuite/kubernetes/service.py
Deployment and Service default timeouts and slow-loadbalancer post-ready wait replaced with DEPLOYMENT_READY_TIMEOUT, SERVICE_DELETE_TIMEOUT, SERVICE_READY_TIMEOUT, and SLOW_LOADBALANCER_WAIT.
Backend services and configuration port mapping
testsuite/backend/*, testsuite/config/__init__.py
Backend modules and Dynaconf defaults now use shared port constants like HTTP_API_PORT, MOCKSERVER_INTERNAL_PORT, OTEL_COLLECTOR_PORT, VAULT_PORT, and REDIS_PORT for Kubernetes Service/Deployment ports and default endpoints.
Gateway and route readiness timing and ports
testsuite/gateway/envoy/*, testsuite/gateway/gateway_api/*
Envoy and Kuadrant gateway and route readiness now use HTTP_API_PORT, ENVOY_ADMIN_PORT, ENVOY_STARTUP_SETTLE, ENVOY_READINESS_*, GATEWAY_READY_TIMEOUT, ROUTE_READY_TIMEOUT, and SLOW_LOADBALANCER_WAIT.
Policy enforcement and ready timeouts
testsuite/kuadrant/policy/*, testsuite/kuadrant/extensions/oidc_policy.py
Policy modules use centralized defaults: POLICY_ENFORCEMENT_TIMEOUT, DNS_POLICY_ENFORCEMENT_TIMEOUT, TLS_POLICY_ENFORCEMENT_TIMEOUT, and post-enforcement waits RLP_POST_ENFORCEMENT_WAIT, OIDC_POST_ENFORCEMENT_WAIT.
HTTP and tracing client backoff/retry configuration
testsuite/httpx/__init__.py, testsuite/tracing/*, testsuite/grpc/__init__.py
HTTP client backoff uses HTTP_BACKOFF_MAX_RETRIES; Jaeger/Tempo tracing retries use TRACING_MAX_RETRIES; gRPC unary calls use GRPC_CALL_TIMEOUT.
Prometheus and SpiceDB client configuration
testsuite/prometheus.py, testsuite/spicedb/spicedb.py
Prometheus polling/retry decorators use PROMETHEUS_* interval/retry constants; SpiceDB client uses SPICEDB_CONNECTION_TIMEOUT, SPICEDB_MAX_RETRIES, SPICEDB_RETRY_INTERVAL, and SPICEDB_*_PORT constants for client and Kubernetes wiring.
Contribution guideline
CONTRIBUTING.md
Adds "Using Constants" guidance directing contributors to centralise shared timeouts/ports/retries in testsuite/utils/constants.py and follow naming conventions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Kuadrant/testsuite#958: Updates GRPCRoute.wait_for_ready() timeout handling; related to replacing hardcoded route timeouts with ROUTE_READY_TIMEOUT.
  • Kuadrant/testsuite#870: Refactors tracing API and related retry logic; related to switching tracing retry decorators to TRACING_MAX_RETRIES.

Suggested labels

refactor, infrastructure

Suggested reviewers

  • silvi-t
  • emmaaroche
  • zkraus

Poem

🐰 I hopped through code both short and long,

Gathered every magic number into one strong song,
Timeouts, ports and retries now dwell in a row,
Neat constants in a file where all can go,
A tidy test suite—hippity-hop, let's go!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 78.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the primary change: replacing hardcoded magic numbers with named constants across the codebase.
Description check ✅ Passed The description comprehensively covers the PR objective, lists main changes, and provides verification results. It follows the template structure with clear sections and detailed information.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@testsuite/utils/constants.py`:
- Line 72: Replace the ambiguous Unicode MULTIPLICATION SIGN in the comment
"SpiceDB relationship readiness retry (5s × 3)" with an ASCII character such as
lowercase 'x' or '*' so the comment reads e.g. "SpiceDB relationship readiness
retry (5s x 3)" to avoid non-ASCII punctuation; update that exact comment string
in testsuite/utils/constants.py.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 36495775-e0da-4fdc-9a7f-d73cc5faad80

📥 Commits

Reviewing files that changed from the base of the PR and between babe841 and ceddf01.

📒 Files selected for processing (25)
  • testsuite/backend/__init__.py
  • testsuite/backend/httpbin.py
  • testsuite/backend/llm_sim.py
  • testsuite/backend/mockserver.py
  • testsuite/config/__init__.py
  • testsuite/gateway/envoy/__init__.py
  • testsuite/gateway/gateway_api/gateway.py
  • testsuite/gateway/gateway_api/grpc_route.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/grpc/__init__.py
  • testsuite/httpx/__init__.py
  • testsuite/kuadrant/extensions/oidc_policy.py
  • testsuite/kuadrant/policy/__init__.py
  • testsuite/kuadrant/policy/dns.py
  • testsuite/kuadrant/policy/rate_limit.py
  • testsuite/kuadrant/policy/tls.py
  • testsuite/kubernetes/__init__.py
  • testsuite/kubernetes/deployment.py
  • testsuite/kubernetes/service.py
  • testsuite/prometheus.py
  • testsuite/spicedb/spicedb.py
  • testsuite/tracing/jaeger.py
  • testsuite/tracing/tempo.py
  • testsuite/utils/__init__.py
  • testsuite/utils/constants.py

Comment thread testsuite/utils/constants.py Outdated
Copy link
Copy Markdown
Contributor

@emmaaroche emmaaroche left a comment

Choose a reason for hiding this comment

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

Great progress on this issue!

I know this PR focused on infrastructure modules, but if you're interested in tackling test files as a follow-up (whether it be this or another PR) here are the remaining magic numbers:

Test files with time.sleep() calls:

  • testsuite/tests/singlecluster/gateway/scaling/test_manual_scale_gateway.py
  • testsuite/tests/singlecluster/gateway/scaling/test_auto_scale_gateway.py
  • testsuite/tests/singlecluster/grpc/test_grpcroute.py
  • testsuite/tests/singlecluster/gateway/reconciliation/change_targetref/test_update_tlspolicy_target_ref.py
  • testsuite/tests/singlecluster/gateway/reconciliation/change_targetref/conftest.py
  • testsuite/tests/singlecluster/authorino/identity/x509/gateway_validation/test_multi_ca_trust.py
  • testsuite/tests/singlecluster/authorino/identity/plain/test_jwt_user_story.py

Test files with direct sleep() calls:

  • testsuite/tests/singlecluster/identical_hostnames/rlp/test_rlp_on_routes.py
  • testsuite/tests/singlecluster/identical_hostnames/rlp/test_rlp_on_gw_and_route.py
  • testsuite/tests/singlecluster/limitador/test_multiple_iterations.py
  • testsuite/tests/singlecluster/limitador/tokens_rate_limit/basic/test_trlp_streaming.py
  • testsuite/tests/singlecluster/limitador/tokens_rate_limit/basic/test_trlp.py
  • testsuite/tests/singlecluster/limitador/tokens_rate_limit/multiple_iterations/test_multiple_trlp_iterations.py
  • testsuite/tests/singlecluster/limitador/tokens_rate_limit/multiple_iterations/test_multiple_trlp_stream_iterations.py
  • testsuite/tests/singlecluster/egress/test_egress.py
  • testsuite/tests/singlecluster/authorino/authorization/opa/external_registry/test_cache.py
  • testsuite/tests/multicluster/load_balanced/test_change_default_geo.py
  • testsuite/tests/multicluster/load_balanced/test_change_strategy.py

Test files with hardcoded timeout/timelimit values:

  • testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_remove_endpoint.py
  • testsuite/tests/singlecluster/gateway/reconciliation/change_targetref/conftest.py
  • testsuite/tests/singlecluster/gateway/reconciliation/test_gw_doesnt_exist.py
  • testsuite/tests/singlecluster/gateway/reconciliation/test_invalid_issuer_reference.py
  • testsuite/tests/singlecluster/gateway/reconciliation/test_same_target.py
  • testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_errors.py
  • testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_lifecycle.py

Test files with hardcoded backoff/retry configs:

  • testsuite/tests/singlecluster/observability/conftest.py

Test fixtures with hardcoded ports:

  • testsuite/tests/singlecluster/egress/credentials_injection/conftest.py
  • testsuite/tests/singlecluster/authorino/wristband/conftest.py
  • testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py

Also left 2 nitpick comments.

Comment thread testsuite/utils/constants.py Outdated
Comment thread testsuite/utils/constants.py Outdated
@silvi-t
Copy link
Copy Markdown
Contributor

silvi-t commented May 15, 2026

Thanks for working on this! It would also be great if this PR included a short section in the contributing docs (e.g. CONTRIBUTING.md) with guidelines on when and how to use constants in the testsuite.

Signed-off-by: Priyanshu-u07 <connect.priyanshu8271@gmail.com>
@Priyanshu-u07 Priyanshu-u07 force-pushed the add-magic-number-constants branch from 1881f2b to 73bb0ac Compare May 15, 2026 16:17
@Priyanshu-u07
Copy link
Copy Markdown
Contributor Author

@emmaaroche, @silvi-t, I've addressed the nitpick comments and also added a "Using Constants" section to CONTRIBUTING.md.

Thanks Emma for the review and for pointing out the test files.
To keep this PR focused on the infrastructure and ensure the constants module is available on main for imports, I’ll work on the test files in a follow-up PR right after this one merges.

Copy link
Copy Markdown
Contributor

@emmaaroche emmaaroche left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM! Please wait for a 2nd reviewer to approve before merging as this PR touches shared infrastructure files 👍

@emmaaroche emmaaroche requested a review from a team May 19, 2026 13:52
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