From 264e2186cd6e57d8767dfaf695adbb39be8d1a36 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Mon, 21 Oct 2024 12:55:05 -0400 Subject: [PATCH 1/2] fix: search_base_query to handle desc ordering --- graphql_api/tests/test_test_analytics.py | 88 ++++++---------------- utils/test_results.py | 19 ++++- utils/tests/unit/test_search_base_query.py | 10 +++ 3 files changed, 47 insertions(+), 70 deletions(-) diff --git a/graphql_api/tests/test_test_analytics.py b/graphql_api/tests/test_test_analytics.py index 8655f68c49..8ac2d80a8b 100644 --- a/graphql_api/tests/test_test_analytics.py +++ b/graphql_api/tests/test_test_analytics.py @@ -598,64 +598,36 @@ def test_desc_failure_rate_ordering_on_test_results_with_after(self) -> None: pass_count=2, fail_count=3, ) - res = self.fetch_test_analytics( - repo.name, - """testResults(ordering: { parameter: FAILURE_RATE, direction: DESC }, first: 1) { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""", - ) - assert res["testResults"] == { - "edges": [ - {"node": {"name": test_2.name, "failureRate": 0.6}}, - ], - "pageInfo": { - "endCursor": base64_encode_string(f"0.6|{test_2.name}"), - "hasNextPage": True, - "hasPreviousPage": False, - "startCursor": base64_encode_string(f"0.6|{test_2.name}"), - }, - "totalCount": 2, - } - - res = self.fetch_test_analytics( - repo.name, - """testResults(ordering: { parameter: FAILURE_RATE, direction: DESC }, first: 1, after: "%s") { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""" - % res["testResults"]["pageInfo"]["endCursor"], + test_3 = TestFactory(repository=repo) + _ = DailyTestRollupFactory( + test=test_3, + date=datetime.date.today(), + repoid=repo.repoid, + pass_count=1, + fail_count=4, ) - - assert res["testResults"] == { - "edges": [ - {"node": {"name": test.name, "failureRate": 0.2}}, - ], - "pageInfo": { - "endCursor": base64_encode_string(f"0.2|{test.name}"), - "hasNextPage": False, - "hasPreviousPage": False, - "startCursor": base64_encode_string(f"0.2|{test.name}"), - }, - "totalCount": 2, - } - res = self.fetch_test_analytics( repo.name, - """testResults(ordering: { parameter: FAILURE_RATE, direction: ASC }, first: 1) { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""", + """testResults(ordering: { parameter: FAILURE_RATE, direction: DESC }, first: 1) { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""", ) assert res["testResults"] == { "edges": [ - {"node": {"name": test.name, "failureRate": 0.2}}, + {"node": {"name": test_3.name, "failureRate": 0.8}}, ], "pageInfo": { - "endCursor": base64_encode_string(f"0.2|{test.name}"), + "endCursor": base64_encode_string(f"0.8|{test_3.name}"), "hasNextPage": True, "hasPreviousPage": False, - "startCursor": base64_encode_string(f"0.2|{test.name}"), + "startCursor": base64_encode_string(f"0.8|{test_3.name}"), }, - "totalCount": 2, + "totalCount": 3, } res = self.fetch_test_analytics( repo.name, - """testResults(ordering: { parameter: FAILURE_RATE, direction: ASC }, first: 1, after: "%s") { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""" + """testResults(ordering: { parameter: FAILURE_RATE, direction: DESC }, first: 1, after: "%s") { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""" % res["testResults"]["pageInfo"]["endCursor"], ) @@ -665,48 +637,30 @@ def test_desc_failure_rate_ordering_on_test_results_with_after(self) -> None: ], "pageInfo": { "endCursor": base64_encode_string(f"0.6|{test_2.name}"), - "hasNextPage": False, + "hasNextPage": True, "hasPreviousPage": False, "startCursor": base64_encode_string(f"0.6|{test_2.name}"), }, - "totalCount": 2, + "totalCount": 3, } res = self.fetch_test_analytics( repo.name, - """testResults(ordering: { parameter: FAILURE_RATE, direction: ASC }, last: 2) { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""", + """testResults(ordering: { parameter: FAILURE_RATE, direction: DESC }, first: 1, after: "%s") { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""" + % res["testResults"]["pageInfo"]["endCursor"], ) assert res["testResults"] == { "edges": [ - {"node": {"name": test_2.name, "failureRate": 0.6}}, {"node": {"name": test.name, "failureRate": 0.2}}, ], "pageInfo": { "endCursor": base64_encode_string(f"0.2|{test.name}"), "hasNextPage": False, "hasPreviousPage": False, - "startCursor": base64_encode_string(f"0.6|{test_2.name}"), - }, - "totalCount": 2, - } - - res = self.fetch_test_analytics( - repo.name, - """testResults(ordering: { parameter: FAILURE_RATE, direction: ASC }, last: 1) { edges { node { name failureRate } }, pageInfo { hasNextPage, hasPreviousPage, startCursor, endCursor }, totalCount }""", - ) - - assert res["testResults"] == { - "edges": [ - {"node": {"name": test_2.name, "failureRate": 0.6}}, - ], - "pageInfo": { - "endCursor": base64_encode_string(f"0.6|{test_2.name}"), - "hasNextPage": False, - "hasPreviousPage": True, - "startCursor": base64_encode_string(f"0.6|{test_2.name}"), + "startCursor": base64_encode_string(f"0.2|{test.name}"), }, - "totalCount": 2, + "totalCount": 3, } def test_flake_rate_filtering_on_test_results(self) -> None: @@ -1184,7 +1138,7 @@ def test_test_suites_no_term(self) -> None: ) res = self.fetch_test_analytics( repo.name, - """testSuites""", + """testSuites(term: "hello")""", ) assert sorted(res["testSuites"]) == ["goodbye_world", "hello_world"] @@ -1218,6 +1172,6 @@ def test_flags_no_term(self) -> None: res = self.fetch_test_analytics( repo.name, - """flags""", + """flags(term: "hello")""", ) assert sorted(res["flags"]) == ["goodbye_world", "hello_world"] diff --git a/utils/test_results.py b/utils/test_results.py index c303016f8b..8797c9c400 100644 --- a/utils/test_results.py +++ b/utils/test_results.py @@ -242,6 +242,7 @@ def search_base_query( rows: list[TestResultsRow], ordering: str, cursor: CursorValue | None, + descending: bool = False, ) -> list[TestResultsRow]: """ The reason we have to do this filtering in the application logic is because we need to get the total count of rows that @@ -270,7 +271,14 @@ def compare(row: TestResultsRow) -> int: row_value = getattr(row, ordering) row_value_str = str(row_value) cursor_value_str = cursor.ordered_value - return (row_value_str > cursor_value_str) - (row_value_str < cursor_value_str) + if descending: + return (row_value_str < cursor_value_str) - ( + row_value_str > cursor_value_str + ) + else: + return (row_value_str > cursor_value_str) - ( + row_value_str < cursor_value_str + ) left, right = 0, len(rows) - 1 while left <= right: @@ -442,8 +450,13 @@ def generate_test_results( page_size: int = first or last or 20 cursor_value = decode_cursor(after) if after else decode_cursor(before) - - search_rows = search_base_query(rows, ordering, cursor_value) + descending = ordering_direction == "DESC" + search_rows = search_base_query( + rows, + ordering, + cursor_value, + descending=descending, + ) page: list[dict[str, str | TestResultsRow]] = [ {"cursor": encode_cursor(row, ordering), "node": row} diff --git a/utils/tests/unit/test_search_base_query.py b/utils/tests/unit/test_search_base_query.py index 4590b57cd1..5ebacd1582 100644 --- a/utils/tests/unit/test_search_base_query.py +++ b/utils/tests/unit/test_search_base_query.py @@ -51,3 +51,13 @@ def test_search_base_query_with_missing_cursor_low_name_high_failure_rate(): cursor = CursorValue(name="0", ordered_value="0.15") res = search_base_query(rows, "failure_rate", cursor) assert res == rows[-1:] + + +def test_search_base_query_with_missing_cursor_low_name_high_failure_rate_desc(): + # [(2, "0.2"), (1, "0.1"), (0, "0.0")] + # ^ + # here's where the cursor is pointing at + rows = [row_factory(str(i), float(i) * 0.1) for i in range(2, -1, -1)] + cursor = CursorValue(name="0", ordered_value="0.15") + res = search_base_query(rows, "failure_rate", cursor, descending=True) + assert res == rows[1:] From 6009eff30cf4c120f5f9fa2b61730cd745c29c58 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Mon, 21 Oct 2024 13:08:50 -0400 Subject: [PATCH 2/2] fix: address feedback --- graphql_api/tests/test_test_analytics.py | 4 ++-- utils/test_results.py | 10 ++++------ utils/tests/unit/test_search_base_query.py | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/graphql_api/tests/test_test_analytics.py b/graphql_api/tests/test_test_analytics.py index 8ac2d80a8b..501dfd8f97 100644 --- a/graphql_api/tests/test_test_analytics.py +++ b/graphql_api/tests/test_test_analytics.py @@ -1138,7 +1138,7 @@ def test_test_suites_no_term(self) -> None: ) res = self.fetch_test_analytics( repo.name, - """testSuites(term: "hello")""", + """testSuites""", ) assert sorted(res["testSuites"]) == ["goodbye_world", "hello_world"] @@ -1172,6 +1172,6 @@ def test_flags_no_term(self) -> None: res = self.fetch_test_analytics( repo.name, - """flags(term: "hello")""", + """flags""", ) assert sorted(res["flags"]) == ["goodbye_world", "hello_world"] diff --git a/utils/test_results.py b/utils/test_results.py index 8797c9c400..8174f9a7e2 100644 --- a/utils/test_results.py +++ b/utils/test_results.py @@ -271,14 +271,12 @@ def compare(row: TestResultsRow) -> int: row_value = getattr(row, ordering) row_value_str = str(row_value) cursor_value_str = cursor.ordered_value + row_is_greater = row_value_str > cursor_value_str + row_is_less = row_value_str < cursor_value_str if descending: - return (row_value_str < cursor_value_str) - ( - row_value_str > cursor_value_str - ) + return row_is_less - row_is_greater else: - return (row_value_str > cursor_value_str) - ( - row_value_str < cursor_value_str - ) + return row_is_greater - row_is_less left, right = 0, len(rows) - 1 while left <= right: diff --git a/utils/tests/unit/test_search_base_query.py b/utils/tests/unit/test_search_base_query.py index 5ebacd1582..0387c0df7a 100644 --- a/utils/tests/unit/test_search_base_query.py +++ b/utils/tests/unit/test_search_base_query.py @@ -53,7 +53,7 @@ def test_search_base_query_with_missing_cursor_low_name_high_failure_rate(): assert res == rows[-1:] -def test_search_base_query_with_missing_cursor_low_name_high_failure_rate_desc(): +def test_search_base_query_descending(): # [(2, "0.2"), (1, "0.1"), (0, "0.0")] # ^ # here's where the cursor is pointing at