diff --git a/cli/bin/postgres-ai.ts b/cli/bin/postgres-ai.ts index 74487a0..7ac5d04 100644 --- a/cli/bin/postgres-ai.ts +++ b/cli/bin/postgres-ai.ts @@ -195,10 +195,6 @@ function createTtySpinner( }; } -// ============================================================================ -// Checkup command helpers -// ============================================================================ - interface CheckupOptions { checkId: string; nodeName: string; @@ -409,13 +405,6 @@ function printUploadSummary( } } -// ============================================================================ -// CLI configuration -// ============================================================================ - -/** - * CLI configuration options - */ interface CliOptions { apiKey?: string; apiBaseUrl?: string; diff --git a/cli/lib/checkup.ts b/cli/lib/checkup.ts index 9e8e992..566bc19 100644 --- a/cli/lib/checkup.ts +++ b/cli/lib/checkup.ts @@ -955,10 +955,6 @@ function resolveBuildTs(): string | null { } } -// ============================================================================ -// Unified Report Generator Helpers -// ============================================================================ - /** * Generate a simple version report (A002, A013). * These reports only contain PostgreSQL version information. @@ -1034,10 +1030,6 @@ async function generateIndexReport( return report; } -// ============================================================================ -// Report Generators (using unified helpers) -// ============================================================================ - /** Generate A002 report - Postgres major version */ export const generateA002 = (client: Client, nodeName = "node-01") => generateVersionReport(client, nodeName, "A002", "Postgres major version"); diff --git a/cli/lib/issues.ts b/cli/lib/issues.ts index e940aaf..d982341 100644 --- a/cli/lib/issues.ts +++ b/cli/lib/issues.ts @@ -670,10 +670,6 @@ export async function updateIssueComment(params: UpdateIssueCommentParams): Prom } } -// ============================================================================ -// Action Items API Functions -// ============================================================================ - export interface FetchActionItemParams { apiKey: string; apiBaseUrl: string; diff --git a/cli/lib/reports.ts b/cli/lib/reports.ts index 55b782c..3a313d2 100644 --- a/cli/lib/reports.ts +++ b/cli/lib/reports.ts @@ -1,9 +1,5 @@ import { formatHttpError, maskSecret, normalizeBaseUrl } from "./util"; -// ============================================================================ -// Types -// ============================================================================ - export interface CheckupReport { id: number; org_id: number; @@ -32,10 +28,6 @@ export interface CheckupReportFileData extends CheckupReportFile { data: string; } -// ============================================================================ -// Date parsing -// ============================================================================ - /** * Parse a date string in various formats into an ISO 8601 string. * Supported formats: @@ -73,10 +65,6 @@ export function parseFlexibleDate(input: string): string { throw new Error(`Unrecognized date format: ${input}. Use YYYY-MM-DD or DD.MM.YYYY`); } -// ============================================================================ -// Params -// ============================================================================ - export interface FetchReportsParams { apiKey: string; apiBaseUrl: string; @@ -107,10 +95,6 @@ export interface FetchReportFileDataParams { debug?: boolean; } -// ============================================================================ -// API functions -// ============================================================================ - export async function fetchReports(params: FetchReportsParams): Promise { const { apiKey, apiBaseUrl, projectId, status, limit = 20, beforeDate, beforeId, debug } = params; if (!apiKey) { @@ -299,10 +283,7 @@ export async function fetchReportFileData(params: FetchReportFileDataParams): Pr } } -// ============================================================================ -// Lightweight markdown terminal renderer -// ============================================================================ - +/** Lightweight markdown terminal renderer. */ export function renderMarkdownForTerminal(md: string): string { if (!md) return ""; diff --git a/cli/test/init.test.ts b/cli/test/init.test.ts index 37004d6..20e36d1 100644 --- a/cli/test/init.test.ts +++ b/cli/test/init.test.ts @@ -1318,12 +1318,8 @@ describe.skipIf(!dockerAvailable)("imageTag priority behavior", () => { }, 60000); }); -// --------------------------------------------------------------------------- -// connectWithSslFallback — connectionTimeoutMillis and statement_timeout -// Issues 9 & 10 -// --------------------------------------------------------------------------- describe("connectWithSslFallback", () => { - // Issue 9: Verify that connectionTimeoutMillis is forwarded to the pg Client + // Verify that connectionTimeoutMillis is forwarded to the pg Client // constructor so slow-responding servers don't hang the CLI indefinitely. // Direct integration testing against a real TCP timeout would be flaky in CI, // so we use a mock ClientClass and assert the config passed to its constructor. @@ -1348,7 +1344,7 @@ describe("connectWithSslFallback", () => { expect(receivedConfigs[0].connectionTimeoutMillis).toBe(10_000); }); - // Issue 10: Verify that SET statement_timeout is issued after every successful + // Verify that SET statement_timeout is issued after every successful // connection to prevent runaway queries from blocking the CLI. test("issues SET statement_timeout = '30s' after connecting", async () => { const queriesSent: string[] = []; diff --git a/cli/test/reports.cli.test.ts b/cli/test/reports.cli.test.ts index 6b7680a..d67da8a 100644 --- a/cli/test/reports.cli.test.ts +++ b/cli/test/reports.cli.test.ts @@ -167,9 +167,6 @@ async function startFakeApi() { }; } -// --------------------------------------------------------------------------- -// Help output -// --------------------------------------------------------------------------- describe("CLI reports command group", () => { test("reports help exposes list, files, data subcommands", () => { const r = runCli(["reports", "--help"], isolatedEnv()); @@ -180,9 +177,6 @@ describe("CLI reports command group", () => { expect(out).toContain("data"); }); - // ----------------------------------------------------------------------- - // Input validation - // ----------------------------------------------------------------------- test("reports list fails fast when API key is missing", () => { const r = runCli(["reports", "list"], isolatedEnv()); expect(r.status).toBe(1); @@ -237,9 +231,6 @@ describe("CLI reports command group", () => { expect(`${r.stdout}\n${r.stderr}`).toContain("Either reportId or --check-id is required"); }); - // ----------------------------------------------------------------------- - // Successful API calls - // ----------------------------------------------------------------------- test("reports list succeeds against a fake API", async () => { const api = await startFakeApi(); try { diff --git a/cli/test/reports.test.ts b/cli/test/reports.test.ts index 4082e53..42ad145 100644 --- a/cli/test/reports.test.ts +++ b/cli/test/reports.test.ts @@ -10,9 +10,6 @@ import { const originalFetch = globalThis.fetch; -// --------------------------------------------------------------------------- -// fetchReports -// --------------------------------------------------------------------------- describe("fetchReports", () => { afterEach(() => { globalThis.fetch = originalFetch; @@ -274,9 +271,6 @@ describe("fetchReports", () => { }); }); -// --------------------------------------------------------------------------- -// fetchAllReports -// --------------------------------------------------------------------------- describe("fetchAllReports", () => { afterEach(() => { globalThis.fetch = originalFetch; @@ -416,9 +410,6 @@ describe("fetchAllReports", () => { }); }); -// --------------------------------------------------------------------------- -// fetchReportFiles -// --------------------------------------------------------------------------- describe("fetchReportFiles", () => { afterEach(() => { globalThis.fetch = originalFetch; @@ -623,9 +614,6 @@ describe("fetchReportFiles", () => { }); }); -// --------------------------------------------------------------------------- -// fetchReportFileData -// --------------------------------------------------------------------------- describe("fetchReportFileData", () => { afterEach(() => { globalThis.fetch = originalFetch; @@ -841,9 +829,6 @@ describe("fetchReportFileData", () => { }); }); -// --------------------------------------------------------------------------- -// renderMarkdownForTerminal -// --------------------------------------------------------------------------- describe("renderMarkdownForTerminal", () => { test("returns empty string for empty input", () => { expect(renderMarkdownForTerminal("")).toBe(""); @@ -923,9 +908,6 @@ describe("renderMarkdownForTerminal", () => { }); }); -// --------------------------------------------------------------------------- -// parseFlexibleDate -// --------------------------------------------------------------------------- describe("parseFlexibleDate", () => { test("parses YYYY-MM-DD", () => { expect(parseFlexibleDate("2025-01-15")).toBe("2025-01-15T00:00:00.000Z"); diff --git a/monitoring_flask_backend/test_app.py b/monitoring_flask_backend/test_app.py index c8f9be1..8117a0b 100644 --- a/monitoring_flask_backend/test_app.py +++ b/monitoring_flask_backend/test_app.py @@ -1877,12 +1877,8 @@ def test_statement_timeout_returns_empty_metrics(self, mock_connect, client): assert 'pgwatch_query_info{' not in data -# --------------------------------------------------------------------------- -# Additional coverage for /execute-query (issues 4, 6, 7, 8) -# --------------------------------------------------------------------------- - class TestExecuteQuerySuccessPath: - """Issue 4 — success-path test: valid SELECT returning real results.""" + """Success-path test: valid SELECT returning real results.""" def test_returns_columns_and_rows(self, client, debug_mode): """A valid SELECT query returns 200 with populated columns and rows.""" @@ -1905,7 +1901,7 @@ def test_returns_columns_and_rows(self, client, debug_mode): class TestExecuteQueryAuthSchemes: - """Issue 6 — non-Bearer auth schemes (Basic, Token) must be rejected.""" + """Non-Bearer auth schemes (Basic, Token) must be rejected.""" def test_basic_auth_rejected(self, client, debug_mode): """Authorization: Basic is not accepted (only Bearer is valid).""" @@ -1941,7 +1937,7 @@ def test_apikey_scheme_rejected(self, client, debug_mode): class TestExecuteQueryMultipleBlockComments: - """Issue 7 — multiple sequential leading block comments before SELECT.""" + """Multiple sequential leading block comments before SELECT.""" def test_two_sequential_block_comments_accepted(self, client, debug_mode): """/* a */ /* b */ SELECT 1 passes the allowlist (both comments stripped).""" @@ -1979,7 +1975,7 @@ def test_three_sequential_block_comments_accepted(self, client, debug_mode): class TestExecuteQueryStatementTimeout: - """Issue 8 — statement timeout during query execution returns 500.""" + """Statement timeout during query execution returns 500.""" def test_statement_timeout_returns_500(self, client, debug_mode): """OperationalError from statement_timeout inside execute-query returns 500.""" @@ -2001,7 +1997,7 @@ def test_statement_timeout_returns_500(self, client, debug_mode): assert 'error' in data def test_line_comment_containing_blocked_keyword_not_blocked(self, client, debug_mode): - """Issue 2 regression: '-- pg_read_file' in a line comment must not trigger blocklist.""" + """Regression: '-- pg_read_file' in a line comment must not trigger blocklist.""" with patch('app.psycopg2.connect') as mock_connect: mock_cursor = MagicMock() mock_cursor.fetchall.return_value = [(1,)] diff --git a/quality/scripts/release-readiness.sh b/quality/scripts/release-readiness.sh index 36421f8..3131d5e 100755 --- a/quality/scripts/release-readiness.sh +++ b/quality/scripts/release-readiness.sh @@ -2,7 +2,6 @@ # shellcheck shell=bash # # Release Readiness Check -# TODO: add bats tests for this script # # Verifies that the codebase is ready for release by checking: # 1. Git status (clean working tree, release-eligible branch) diff --git a/reporter/postgres_reports.py b/reporter/postgres_reports.py index 86e757c..8a3d0fe 100644 --- a/reporter/postgres_reports.py +++ b/reporter/postgres_reports.py @@ -346,24 +346,20 @@ def get_queryid_queries_from_sink(self, query_text_limit: int = 655360, db_names """ cursor.execute(query) - # Use iterator to fetch rows in batches instead of loading all at once for row in cursor: db_name = row['dbname'] queryid = row['queryid'] query_text = row['query'] - - # Skip if queryid is missing + if not queryid: continue - - # Truncate query text if it exceeds the limit + if query_text and len(query_text) > query_text_limit: query_text = query_text[:query_text_limit] + '...' - - # Initialize database dict if needed + if db_name not in queries_by_db: queries_by_db[db_name] = {} - + queries_by_db[db_name][queryid] = query_text or '' except Exception as e: @@ -639,22 +635,9 @@ def generate_a007_altered_settings_report(self, cluster: str = "local", node_nam return self.format_report_data("A007", altered_settings, node_name, postgres_version=self._get_postgres_version_info(cluster, node_name)) - # ================================================================================== - # H001: Invalid Indexes - Observation Data for Decision Tree - # ================================================================================== - # - # This report collects observation data that enables decision tree analysis: - # - has_valid_duplicate / valid_duplicate_name: Is there a valid index on same column(s)? - # - is_pk / is_unique / constraint_name: Does it back a constraint (UNIQUE / PK)? - # - table_row_estimate: Is the table small (< 10K rows)? - # - # The JSON report contains ONLY observations (raw data). - # Recommendations (DROP, RECREATE, UNCERTAIN) are computed at render time: - # - CLI: cli/lib/checkup.ts -> getInvalidIndexRecommendation() - # - UI: Grafana dashboard or web template applies the same logic - # - # ================================================================================== - + # H001 collects observation data only; recommendations (DROP, RECREATE, UNCERTAIN) + # are computed at render time by CLI (cli/lib/checkup.ts -> getInvalidIndexRecommendation()) + # and UI (Grafana / web template) using the same decision tree. def generate_h001_invalid_indexes_report(self, cluster: str = "local", node_name: str = "node-01") -> Dict[ str, Any]: """ diff --git a/tests/monitoring_flask_backend/test_promql_escape_integration.py b/tests/monitoring_flask_backend/test_promql_escape_integration.py index 526254f..b2d12ea 100644 --- a/tests/monitoring_flask_backend/test_promql_escape_integration.py +++ b/tests/monitoring_flask_backend/test_promql_escape_integration.py @@ -15,10 +15,6 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..', 'monitoring_flask_backend')) -# --------------------------------------------------------------------------- -# Shared helpers -# --------------------------------------------------------------------------- - def _make_prom_mock(query_results=None): """Return a mock PrometheusConnect with empty results by default.""" mock_prom = MagicMock() @@ -41,10 +37,6 @@ def _captured_queries(mock_prom): return queries -# --------------------------------------------------------------------------- -# Fixtures -# --------------------------------------------------------------------------- - @pytest.fixture(autouse=True) def isolate_app_module(): """Force a fresh import of app for each test to avoid cross-test state.""" @@ -63,10 +55,6 @@ def client(): yield c -# =========================================================================== -# get_pgss_metrics_csv (/pgss_metrics/csv) -# =========================================================================== - class TestPgssMetricsCsvPromqlEscape: """Verify that /pgss_metrics/csv correctly escapes PromQL parameters.""" @@ -163,10 +151,6 @@ def test_node_name_newline_escaped(self, client): f"Raw newline found in PromQL query: {q!r}") -# =========================================================================== -# get_btree_bloat_csv (/btree_bloat/csv) -# =========================================================================== - class TestBtreeBloatCsvPromqlEscape: """Verify that /btree_bloat/csv correctly escapes PromQL parameters.""" @@ -254,10 +238,6 @@ def test_node_name_quote_escaped(self, client): f"Unescaped quote breaks label matcher: {q!r}") -# =========================================================================== -# get_table_info_csv (/table_info/csv) -# =========================================================================== - class TestTableInfoCsvPromqlEscape: """Verify that /table_info/csv correctly escapes PromQL parameters.""" diff --git a/tests/monitoring_flask_backend/test_query_texts_endpoint.py b/tests/monitoring_flask_backend/test_query_texts_endpoint.py index 54fe163..fcafec1 100644 --- a/tests/monitoring_flask_backend/test_query_texts_endpoint.py +++ b/tests/monitoring_flask_backend/test_query_texts_endpoint.py @@ -40,10 +40,6 @@ def _create_mock(rows=None): return _create_mock - # ========================================================================= - # Response Format Tests - # ========================================================================= - def test_returns_200_ok(self, client, mock_db_connection): """Test that /query_texts endpoint returns 200 status.""" mock_conn, _ = mock_db_connection([]) @@ -149,10 +145,6 @@ def test_multiple_results(self, client, mock_db_connection): assert '222' in queryids assert '333' in queryids - # ========================================================================= - # Truncate Parameter Tests - # ========================================================================= - def test_default_truncate_is_40(self, client, mock_db_connection): """ Test that default truncate length is 40 characters. @@ -300,10 +292,6 @@ def test_negative_truncate(self, client, mock_db_connection): # but should still return 200 assert response.status_code == 200 - # ========================================================================= - # db_name Filter Logic Tests - # ========================================================================= - def test_no_db_name_filter_queries_all(self, client, mock_db_connection): """Test that no db_name parameter queries all databases.""" rows = [ @@ -436,10 +424,6 @@ def test_db_name_grafana_variable_complex_skips_filter(self, client, mock_db_con query = call_args[0][0] assert 'dbname = %s' not in query - # ========================================================================= - # Smart Truncation Behavior Tests - # ========================================================================= - def test_select_query_smart_truncation(self, client, mock_db_connection): """Test that SELECT queries are truncated smartly to show FROM clause.""" long_query = 'SELECT id, name, email, created_at, updated_at, status FROM users WHERE active = true' @@ -621,10 +605,6 @@ def test_query_with_leading_comment_stripped(self, client, mock_db_connection): # Comment should not be in the result assert 'long comment' not in item['query_text'] - # ========================================================================= - # Error Handling Tests - # ========================================================================= - def test_db_connection_failure_returns_empty_array(self, client): """Test that database connection failure returns empty array gracefully.""" with patch('psycopg2.connect', side_effect=Exception('Connection refused')): @@ -670,10 +650,6 @@ def test_cursor_iteration_failure_returns_empty_array(self, client): data = response.get_json() assert data == [] - # ========================================================================= - # HTTP Method Tests - # ========================================================================= - def test_post_not_allowed(self, client): """Test that POST to /query_texts returns 405.""" response = client.post('/query_texts') @@ -689,10 +665,6 @@ def test_delete_not_allowed(self, client): response = client.delete('/query_texts') assert response.status_code == 405 - # ========================================================================= - # Edge Cases - # ========================================================================= - def test_null_queryid_is_skipped(self, client, mock_db_connection): """Test that rows with null queryid are skipped.""" rows = [ diff --git a/tests/reporter/test_generators_unit.py b/tests/reporter/test_generators_unit.py index 6f357b5..9070ade 100644 --- a/tests/reporter/test_generators_unit.py +++ b/tests/reporter/test_generators_unit.py @@ -1706,11 +1706,6 @@ def test_connection(self) -> bool: postgres_reports_module.main() -# ============================================================================ -# Negative test cases - Error handling -# ============================================================================ - - @pytest.mark.unit def test_query_instant_handles_http_404_error(monkeypatch: pytest.MonkeyPatch, generator: PostgresReportGenerator) -> None: """Test that query_instant returns empty dict on HTTP 404 error.""" diff --git a/tests/reporter/test_promql_escaping.py b/tests/reporter/test_promql_escaping.py index 4fb0ccc..9a60f69 100644 --- a/tests/reporter/test_promql_escaping.py +++ b/tests/reporter/test_promql_escaping.py @@ -9,10 +9,8 @@ import pytest -# --------------------------------------------------------------------------- # Mock unavailable Flask-stack dependencies so the pure escape helpers can be # imported without a running Flask application or installed packages. -# --------------------------------------------------------------------------- for _mod in [ "flask", "prometheus_api_client", @@ -102,10 +100,6 @@ def generator(): ) -# --------------------------------------------------------------------------- -# reporter.postgres_reports._escape_promql_label -# --------------------------------------------------------------------------- - class TestEscapePromqlLabel: """Tests for PostgresReportGenerator._escape_promql_label static method.""" @@ -168,10 +162,6 @@ def test_parameterized_injection(self, payload, expected): assert PostgresReportGenerator._escape_promql_label(payload) == expected -# --------------------------------------------------------------------------- -# monitoring_flask_backend.app.escape_promql_label -# --------------------------------------------------------------------------- - class TestFlaskEscapePromqlLabel: """Tests for the Flask app's module-level escape_promql_label function.""" @@ -230,10 +220,6 @@ def test_parameterized_injection(self, payload, expected): assert flask_escape_promql_label(payload) == expected -# --------------------------------------------------------------------------- -# monitoring_flask_backend.app.escape_promql_regex_literal -# --------------------------------------------------------------------------- - class TestFlaskEscapePromqlRegexLiteral: """Tests for the Flask app's escape_promql_regex_literal function. @@ -320,10 +306,6 @@ def test_parameterized_injection(self, payload, expected): assert flask_escape_promql_regex_literal(payload) == expected -# --------------------------------------------------------------------------- -# reporter.postgres_reports.PostgresReportGenerator — table bloat queries -# --------------------------------------------------------------------------- - class TestGetTableBloatEscaping: """Tests that generate_f004_heap_bloat_report properly escapes user-supplied parameters (cluster, node_name) in the PromQL queries it builds.