fix: address feedback

Author: joseph-sentry

graphql_api/types/flake_aggregates/flake_aggregates.py

@@ -5,6 +5,7 @@
 from graphql import GraphQLResolveInfo
 from shared.django_apps.core.models import Repository
 
+from graphql_api.types.enums.enum_types import MeasurementInterval
 from utils.test_results import get_results
 
 
@@ -48,13 +49,17 @@ def flake_aggregates_with_percentage(
     return FlakeAggregates(**aggregates)
 
 
-def generate_flake_aggregates(repoid: int, interval: int) -> FlakeAggregates | None:
+def generate_flake_aggregates(
+    repoid: int, interval: MeasurementInterval
+) -> FlakeAggregates | None:
     repo = Repository.objects.get(repoid=repoid)
 
-    curr_results = get_results(repo.repoid, repo.branch, interval)
+    curr_results = get_results(repo.repoid, repo.branch, interval.value)
     if curr_results is None:
         return None
-    past_results = get_results(repo.repoid, repo.branch, interval * 2, interval)
+    past_results = get_results(
+        repo.repoid, repo.branch, interval.value * 2, interval.value
+    )
     if past_results is None:
         return flake_aggregates_from_table(curr_results)
     else:

graphql_api/types/test_analytics/test_analytics.py

@@ -29,6 +29,10 @@
 
 log = logging.getLogger(__name__)
 
+INTERVAL_30_DAY = 30
+INTERVAL_7_DAY = 7
+INTERVAL_1_DAY = 1
+
 
 @dataclass
 class TestResultsRow:
@@ -109,7 +113,7 @@ def validate(
     first: int | None,
     last: int | None,
 ) -> None:
-    if interval not in {1, 7, 30}:
+    if interval not in {INTERVAL_1_DAY, INTERVAL_7_DAY, INTERVAL_30_DAY}:
         raise ValidationError(f"Invalid interval: {interval}")
 
     if not isinstance(ordering_direction, OrderingDirection):
@@ -125,6 +129,22 @@ def validate(
         raise ValidationError("After and before can not be used at the same time")
 
 
+def ordering_expression(
+    ordering: TestResultsOrderingParameter, cursor_value: CursorValue, is_forward: bool
+) -> pl.Expr:
+    if is_forward:
+        ordering_expression = (pl.col(ordering.value) > cursor_value.ordered_value) | (
+            (pl.col(ordering.value) == cursor_value.ordered_value)
+            & (pl.col("name") > cursor_value.name)
+        )
+    else:
+        ordering_expression = (pl.col(ordering.value) < cursor_value.ordered_value) | (
+            (pl.col(ordering.value) == cursor_value.ordered_value)
+            & (pl.col("name") > cursor_value.name)
+        )
+    return ordering_expression
+
+
 def generate_test_results(
     ordering: TestResultsOrderingParameter,
     ordering_direction: OrderingDirection,
@@ -182,7 +202,8 @@ def generate_test_results(
 
     if flags:
         table = table.filter(
-            pl.col("flags").list.eval(pl.element().is_in(flags)).list.any()
+            pl.col("flags").is_not_null()
+            & pl.col("flags").list.eval(pl.element().is_in(flags)).list.any()
         )
 
     match parameter:
@@ -201,23 +222,6 @@ def generate_test_results(
 
     total_count = table.height
 
-    def ordering_expression(cursor_value: CursorValue, is_forward: bool) -> pl.Expr:
-        if is_forward:
-            ordering_expression = (
-                pl.col(ordering.value) > cursor_value.ordered_value
-            ) | (
-                (pl.col(ordering.value) == cursor_value.ordered_value)
-                & (pl.col("name") > cursor_value.name)
-            )
-        else:
-            ordering_expression = (
-                pl.col(ordering.value) < cursor_value.ordered_value
-            ) | (
-                (pl.col(ordering.value) == cursor_value.ordered_value)
-                & (pl.col("name") > cursor_value.name)
-            )
-        return ordering_expression
-
     if after:
         if ordering_direction == OrderingDirection.ASC:
             is_forward = True
@@ -226,7 +230,9 @@ def ordering_expression(cursor_value: CursorValue, is_forward: bool) -> pl.Expr:
 
         cursor_value = decode_cursor(after, ordering)
         if cursor_value:
-            table = table.filter(ordering_expression(cursor_value, is_forward))
+            table = table.filter(
+                ordering_expression(ordering, cursor_value, is_forward)
+            )
     elif before:
         if ordering_direction == OrderingDirection.DESC:
             is_forward = True
@@ -235,7 +241,9 @@ def ordering_expression(cursor_value: CursorValue, is_forward: bool) -> pl.Expr:
 
         cursor_value = decode_cursor(before, ordering)
         if cursor_value:
-            table = table.filter(ordering_expression(cursor_value, is_forward))
+            table = table.filter(
+                ordering_expression(ordering, cursor_value, is_forward)
+            )
 
     table = table.sort(
         [ordering.value, "name"],
@@ -363,7 +371,8 @@ async def resolve_test_results_aggregates(
     **_: Any,
 ) -> TestResultsAggregates | None:
     return await sync_to_async(generate_test_results_aggregates)(
-        repoid=repository.repoid, interval=interval.value if interval else 30
+        repoid=repository.repoid,
+        interval=interval if interval else MeasurementInterval.INTERVAL_30_DAY,
     )
 
 
@@ -375,7 +384,8 @@ async def resolve_flake_aggregates(
     **_: Any,
 ) -> FlakeAggregates | None:
     return await sync_to_async(generate_flake_aggregates)(
-        repoid=repository.repoid, interval=interval.value if interval else 30
+        repoid=repository.repoid,
+        interval=interval if interval else MeasurementInterval.INTERVAL_30_DAY,
     )
 
 

graphql_api/types/test_results_aggregates/test_results_aggregates.py

@@ -6,6 +6,7 @@
 from shared.django_apps.core.models import Repository
 
 from utils.test_results import get_results
+from graphql_api.types.enums.enum_types import MeasurementInterval
 
 
 @dataclass
@@ -43,9 +44,11 @@ def calculate_aggregates(table: pl.DataFrame) -> pl.DataFrame:
         ),
         (pl.col("total_skip_count").sum()).alias("skips"),
         (pl.col("total_fail_count").sum()).alias("fails"),
-        ((pl.col("avg_duration") >= pl.col("avg_duration").quantile(0.95)).sum()).alias(
-            "total_slow_tests"
-        ),
+        (
+            (pl.col("avg_duration") >= pl.col("avg_duration").quantile(0.95))
+            .top_k(100)
+            .sum()
+        ).alias("total_slow_tests"),
     )
 
 
@@ -65,6 +68,8 @@ def test_results_aggregates_with_percentage(
 
     merged_results: pl.DataFrame = pl.concat([past_aggregates, curr_aggregates])
 
+    # with_columns upserts the new columns, so if the name already exists it get overwritten
+    # otherwise it's just added
     merged_results = merged_results.with_columns(
         pl.all().pct_change().name.suffix("_percent_change")
     )
@@ -74,14 +79,16 @@ def test_results_aggregates_with_percentage(
 
 
 def generate_test_results_aggregates(
-    repoid: int, interval: int
+    repoid: int, interval: MeasurementInterval
 ) -> TestResultsAggregates | None:
     repo = Repository.objects.get(repoid=repoid)
 
-    curr_results = get_results(repo.repoid, repo.branch, interval)
+    curr_results = get_results(repo.repoid, repo.branch, interval.value)
     if curr_results is None:
         return None
-    past_results = get_results(repo.repoid, repo.branch, interval * 2, interval)
+    past_results = get_results(
+        repo.repoid, repo.branch, interval.value * 2, interval.value
+    )
     if past_results is None:
         return test_results_aggregates_from_table(curr_results)
     else: