Implement PR review suggestions

backend/test_observer/controllers/artefacts/models.py

@@ -60,7 +60,7 @@ class TestExecutionDTO(BaseModel):
     c3_link: str | None
     environment: EnvironmentDTO
     status: TestExecutionStatus
-    test_results: list[TestResult] | None
+    test_results: list[TestResult]
 
 
 class ArtefactBuildDTO(BaseModel):

backend/test_observer/external_apis/c3/c3.py

@@ -5,7 +5,7 @@
 import requests
 from requests import Request, Response
 
-from .models import Report, SubmissionStatus, TestResult
+from .models import Report, SubmissionStatus
 
 logger = logging.getLogger("test-observer-backend")
 
@@ -21,7 +21,6 @@ def get_submissions_statuses(
     ) -> dict[int, SubmissionStatus]:
         str_ids = [str(status_id) for status_id in ids]
 
-        # Edge case: if there are 0 submissions, we don't need to make an API call
         if not str_ids:
             return {}
 
@@ -43,7 +42,6 @@ def get_submissions_statuses(
     def get_reports(self, ids: Iterable[int]) -> dict[int, Report]:
         str_ids = [str(report_id) for report_id in ids]
 
-        # Edge case: if there are 0 reports, we don't need to make an API call
         if not str_ids:
             return {}
 
@@ -57,25 +55,7 @@ def get_reports(self, ids: Iterable[int]) -> dict[int, Report]:
 
         if response.ok:
             reports = response.json()["results"]
-            return {
-                json["id"]: Report(
-                    id=json["id"],
-                    failed_test_count=json["failed_test_count"],
-                    test_count=json["test_count"],
-                    test_results=[
-                        TestResult(
-                            id=test_result["test"]["id"],
-                            name=test_result["test"]["name"],
-                            status=test_result["status"],
-                            type=test_result["test"]["type"],
-                            io_log=test_result["io_log"],
-                            comment=test_result["comment"],
-                        )
-                        for test_result in json["testresult_set"]
-                    ],
-                )
-                for json in reports
-            }
+            return {json["id"]: Report(**json) for json in reports}
         else:
             logger.warning(response.text)
             return {}

backend/test_observer/external_apis/c3/models.py

@@ -1,6 +1,6 @@
 from enum import Enum
 
-from pydantic import BaseModel
+from pydantic import AliasPath, BaseModel, Field
 
 
 class SubmissionProcessingStatus(str, Enum):
@@ -14,17 +14,21 @@ class SubmissionStatus(BaseModel):
     report_id: int | None
 
 
+class TestResultStatus(str, Enum):
+    PASS = "pass"
+    FAIL = "fail"
+    SKIP = "skip"
+
+
 class TestResult(BaseModel):
-    id: int
-    name: str
-    type: str
-    status: str
+    id: int = Field(validation_alias=AliasPath("test", "id"))
+    name: str = Field(validation_alias=AliasPath("test", "name"))
+    status: TestResultStatus
     comment: str
     io_log: str
 
 
 class Report(BaseModel):
     id: int
     failed_test_count: int
-    test_count: int
-    test_results: list[TestResult]
+    test_results: list[TestResult] = Field(validation_alias="testresult_set")

backend/tests/controllers/artefacts/test_artefacts.py

@@ -180,7 +180,10 @@ def test_correct_test_execution_status(
         "get_submissions_statuses",
         lambda *_: {submission_status.id: submission_status},
     )
-    report = Report(id=237670, failed_test_count=0, test_count=0, test_results=[])
+
+    # The type checker cannot recognize that the testresult_set is an alias
+    # to the test_results field, we ignore the call-arg error because of this
+    report = Report(id=237670, failed_test_count=0, testresult_set=[]) # type: ignore
     monkeypatch.setattr(c3api_mock, "get_reports", lambda *_: {report.id: report})
     app.dependency_overrides[C3Api] = c3api_mock
 

backend/tests/external_apis/test_c3.py

@@ -12,6 +12,7 @@
     Report,
     SubmissionProcessingStatus,
     SubmissionStatus,
+    TestResultStatus,
 )
 
 
@@ -21,6 +22,25 @@ class StatusDict(TypedDict):
     status: SubmissionProcessingStatus
 
 
+class C3TestCaseDict(TypedDict):
+    id: int
+    name: str
+
+
+class C3TestResultDict(TypedDict):
+    id: int
+    comment: str
+    io_log: str
+    test: C3TestCaseDict
+    status: str
+
+
+class C3ReportSummaryAPIResponse(TypedDict):
+    id: int
+    failed_test_count: int
+    testresult_set: list[C3TestResultDict]
+
+
 @pytest.fixture
 def prepare_c3api(
     requests_mock: Mocker, monkeypatch: pytest.MonkeyPatch
@@ -111,10 +131,9 @@ def test_get_submissions_statuses(
 
 def test_get_reports(requests_mock: Mocker, prepare_c3api: tuple[C3Api, str]):
     c3, bearer_token = prepare_c3api
-    c3_api_response = {
+    c3_api_response: C3ReportSummaryAPIResponse = {
         "id": 237670,
         "failed_test_count": 0,
-        "test_count": 0,
         "testresult_set": [],
     }
     requests_mock.get(
@@ -123,22 +142,68 @@ def test_get_reports(requests_mock: Mocker, prepare_c3api: tuple[C3Api, str]):
         json={"results": [c3_api_response]},
     )
 
+    reports = c3.get_reports([c3_api_response["id"]])
+
+    # The type checker cannot recognize that the testresult_set is an alias
+    # to the test_results field, we ignore the call-arg error because of this
+    expected_report = Report(**c3_api_response)  # type: ignore
+
+    assert reports == {expected_report.id: expected_report}
+
+
+def test_get_reports_with_test_results(
+    requests_mock: Mocker, prepare_c3api: tuple[C3Api, str]
+):
+    c3, bearer_token = prepare_c3api
+    c3_api_response: C3ReportSummaryAPIResponse = {
+        "id": 237670,
+        "failed_test_count": 0,
+        "testresult_set": [
+            {
+                "id": 123123,
+                "comment": "Test comment",
+                "io_log": "IO log of the test run",
+                "status": "pass",
+                "test": {
+                    "id": 12,
+                    "name": "wireless",
+                },
+            },
+            {
+                "id": 123124,
+                "comment": "Test comment",
+                "io_log": "IO log of the test run",
+                "status": "fail",
+                "test": {
+                    "id": 12,
+                    "name": "camera",
+                },
+            },
+        ],
+    }
+
     requests_mock.get(
-        f"https://certification.canonical.com/api/v2/reports/summary/{c3_api_response['id']}/",
+        f"https://certification.canonical.com/api/v2/reports/summary/?id__in={c3_api_response['id']}&limit=1",
         request_headers={"Authorization": f"Bearer {bearer_token}"},
         json={"results": [c3_api_response]},
     )
 
-    # We ignore this typing error, it happens because of the
-    # "testresult_set" assigned to empty list in the c3_api_response
-    # dict, and the Python Typing system cannot be sure that the "id"
-    # field is int
-    reports = c3.get_reports([c3_api_response["id"]])  # type: ignore
-
-    expected_report = Report(
-        id=237670,
-        failed_test_count=0,
-        test_count=0,
-        test_results=[],
-    )
-    assert reports == {expected_report.id: expected_report}
+    reports = c3.get_reports([c3_api_response["id"]])
+
+    assert len(reports[237670].test_results) == 2
+
+    count_pass = 0
+    count_fail = 0
+    for test_result in reports[237670].test_results:
+        if test_result.status == TestResultStatus.PASS:
+            count_pass += 1
+        elif test_result.status == TestResultStatus.FAIL:
+            count_fail += 1
+
+    assert count_pass == 1 and count_fail == 1
+
+    # The type checker cannot recognize that the testresult_set is an alias
+    # to the test_results field, we ignore the call-arg error because of this
+    expected_report = Report(**c3_api_response)  # type: ignore
+
+    assert reports == {expected_report.id: expected_report}

frontend/lib/models/test_execution.dart

@@ -17,7 +17,7 @@ class TestExecution with _$TestExecution {
     @JsonKey(name: 'c3_link') required String? c3Link,
     required TestExecutionStatus status,
     required Environment environment,
-    @JsonKey(name: 'test_results') required List<TestResult>? testResults,
+    @JsonKey(name: 'test_results') required List<TestResult> testResults,
   }) = _TestExecution;
 
   factory TestExecution.fromJson(Map<String, Object?> json) =>

frontend/lib/models/test_results.dart

@@ -11,7 +11,6 @@ class TestResult with _$TestResult {
   const factory TestResult({
     required int id,
     required String name,
-    required String type,
     required TestResultStatus status,
     @JsonKey(name: 'io_log') required String ioLog,
     required String comment,

frontend/lib/ui/dashboard/artefact_dialog.dart

@@ -277,21 +277,8 @@ class _TestExecutionView extends StatelessWidget {
     final c3Link = testExecution.c3Link;
 
     // Split the test result in separate categories based on Status (PASS/FAIL/SKIP)
-    final Map<TestResultStatus, List<TestResult>> testResultGroups = {};
-    for (var value in TestResultStatus.values) {
-      final List<TestResult> currentTestCases = testExecution.testResults
-              ?.where(
-                (testResult) => testResult.status == value,
-              )
-              .toList() ??
-          [];
-
-      if (currentTestCases.isEmpty) {
-        continue;
-      }
-
-      testResultGroups[value] = currentTestCases;
-    }
+    final Map<TestResultStatus, List<TestResult>> testResultGroups =
+        testExecution.testResults.groupBy(((testResult) => testResult.status));
 
     return YaruExpandable(
       header: Row(

frontend/lib/ui/dashboard/testresults_components.dart

@@ -62,43 +62,47 @@ class _TestResultView extends StatelessWidget {
           Text(
             testResult.name,
             style: Theme.of(context).textTheme.titleLarge,
+            overflow: TextOverflow.ellipsis,
           ),
           const SizedBox(width: Spacing.level4),
         ],
       ),
       expandButtonPosition: YaruExpandableButtonPosition.start,
       child: Padding(
         padding: const EdgeInsets.only(left: Spacing.level6),
-        child: Wrap(
-          children: [
-            testResult.comment != ''
-                ? Wrap(
+        child: Align(
+          alignment: Alignment.topLeft,
+          child: Column(
+            crossAxisAlignment: CrossAxisAlignment.start,
+            children: [
+              if (testResult.comment != '')
+                RichText(
+                  textAlign: TextAlign.left,
+                  text: TextSpan(
                     children: [
-                      Text(
-                        'Test Comment:',
+                      TextSpan(
+                        text: 'Test Comment:',
                         style: Theme.of(context).textTheme.titleMedium,
                       ),
-                      Text(
-                        testResult.comment,
-                      ),
+                      TextSpan(text: testResult.comment),
                     ],
-                  )
-                : const SizedBox.shrink(),
-            testResult.ioLog != ''
-                ? Wrap(
-                    alignment: WrapAlignment.start,
+                  ),
+                ),
+              if (testResult.ioLog != '')
+                RichText(
+                  textAlign: TextAlign.left,
+                  text: TextSpan(
                     children: [
-                      Text(
-                        'Test IO Log:',
+                      TextSpan(
+                        text: 'Test IO Log:',
                         style: Theme.of(context).textTheme.titleMedium,
                       ),
-                      Text(
-                        testResult.ioLog,
-                      ),
+                      TextSpan(text: testResult.ioLog),
                     ],
-                  )
-                : const SizedBox.shrink(),
-          ],
+                  ),
+                ),
+            ],
+          ),
         ),
       ),
     );