From b98852b62a1569c979ac7eb8b837cb6b38187180 Mon Sep 17 00:00:00 2001
From: Paul Clue <67766160+Paul-Clue@users.noreply.github.com>
Date: Wed, 22 Nov 2023 15:48:57 -0500
Subject: [PATCH] No longer mark test plan reports with skipped tests as final
 (#833)

* Hide mark as final button

* No mark as final for skipped tests in graphql

* Remove skipped tests from the UI

* Add migration for unfinished tests

* Simplify necessary files

* Fix failing tests

* Add code review fixes

* Remove no-longer-necessary style class

* Remove reference
---
 client/components/Reports/Reports.css         | 31 ---------
 .../Reports/SummarizeTestPlanReport.jsx       | 26 -------
 .../Reports/SummarizeTestPlanVersion.jsx      | 18 -----
 client/components/TestQueueRow/index.jsx      |  8 ++-
 client/components/TestRun/TestRun.css         |  2 +-
 .../20231031162340-verifyNoSkippedTests.js    | 63 +++++++++++++++++
 .../finalizedTestResultsResolver.js           | 27 +++-----
 .../markAsFinalResolver.js                    | 18 ++---
 server/resolvers/testPlanReportsResolver.js   |  9 +--
 server/scripts/populate-test-data/index.js    |  6 +-
 .../populateFakeTestResults.js                | 69 +++++++++++++------
 server/tests/integration/graphql.test.js      |  4 +-
 12 files changed, 144 insertions(+), 137 deletions(-)
 create mode 100644 server/migrations/20231031162340-verifyNoSkippedTests.js

diff --git a/client/components/Reports/Reports.css b/client/components/Reports/Reports.css
index e6c96b768..2a3768dbd 100644
--- a/client/components/Reports/Reports.css
+++ b/client/components/Reports/Reports.css
@@ -56,36 +56,6 @@
     margin-top: 1em;
 }
 
-.skipped-tests-heading {
-    background: #fefbea;
-    padding: 1.75em 1.75em 1em 1.75em;
-    border: 1px solid #eddfbe;
-    border-bottom: 0;
-    margin-top: 2em;
-}
-
-.skipped-tests {
-    columns: 2;
-    line-height: 2;
-    border: 1px solid #eddfbe;
-    padding: 1.5em 3em;
-    background: #fdfcf6;
-}
-
-.skipped-tests li {
-    margin-bottom: 0.5em;
-    border-bottom: 1px solid #eddfbe;
-    padding-bottom: 0.5em;
-    margin-right: 3em;
-    -webkit-column-break-inside: avoid;
-    page-break-inside: avoid;
-    break-inside: avoid;
-}
-
-.skipped-tests li:last-child {
-    border-bottom: none;
-}
-
 .test-result-heading {
     display: flex;
     justify-content: space-between;
@@ -100,7 +70,6 @@
     margin-bottom: 0;
 }
 
-.skipped-tests-heading h2,
 .test-result-heading h2 {
     margin-top: 0;
 }
diff --git a/client/components/Reports/SummarizeTestPlanReport.jsx b/client/components/Reports/SummarizeTestPlanReport.jsx
index ad6d6542c..0b21f494c 100644
--- a/client/components/Reports/SummarizeTestPlanReport.jsx
+++ b/client/components/Reports/SummarizeTestPlanReport.jsx
@@ -10,7 +10,6 @@ import {
     faExternalLinkAlt,
     faHome
 } from '@fortawesome/free-solid-svg-icons';
-import { differenceBy } from 'lodash';
 import { convertDateToString } from '../../utils/formatter';
 import DisclaimerInfo from '../DisclaimerInfo';
 import TestPlanResultsTable from '../common/TestPlanResultsTable';
@@ -100,11 +99,6 @@ const SummarizeTestPlanReport = ({ testPlanVersion, testPlanReports }) => {
         browser
     };
 
-    const skippedTests = differenceBy(
-        testPlanReport.runnableTests,
-        testPlanReport.finalizedTestResults,
-        testOrTestResult => testOrTestResult.test?.id ?? testOrTestResult.id
-    );
     return (
         <Container id="main" as="main" tabIndex="-1">
             <Helmet>
@@ -269,26 +263,6 @@ const SummarizeTestPlanReport = ({ testPlanVersion, testPlanReports }) => {
                     </Fragment>
                 );
             })}
-            {skippedTests.length ? (
-                <Fragment>
-                    <div className="skipped-tests-heading">
-                        <h2 id="skipped-tests" tabIndex="-1">
-                            Skipped Tests
-                        </h2>
-                        <p>
-                            The following tests have been skipped in this test
-                            run:
-                        </p>
-                    </div>
-                    <ol className="skipped-tests">
-                        {skippedTests.map(test => (
-                            <li key={test.id}>
-                                <a href={test.renderedUrl}>{test.title}</a>
-                            </li>
-                        ))}
-                    </ol>
-                </Fragment>
-            ) : null}
         </Container>
     );
 };
diff --git a/client/components/Reports/SummarizeTestPlanVersion.jsx b/client/components/Reports/SummarizeTestPlanVersion.jsx
index 6cdedc1b6..e9528f4ca 100644
--- a/client/components/Reports/SummarizeTestPlanVersion.jsx
+++ b/client/components/Reports/SummarizeTestPlanVersion.jsx
@@ -1,7 +1,6 @@
 import React, { Fragment } from 'react';
 import PropTypes from 'prop-types';
 import { Helmet } from 'react-helmet';
-import { differenceBy } from 'lodash';
 import getMetrics from './getMetrics';
 import { getTestPlanTargetTitle, getTestPlanVersionTitle } from './getTitles';
 import { Breadcrumb, Button, Container, Table } from 'react-bootstrap';
@@ -86,12 +85,6 @@ const SummarizeTestPlanVersion = ({ testPlanVersion, testPlanReports }) => {
 
             {testPlanReports.map(testPlanReport => {
                 if (testPlanReport.status === 'DRAFT') return null;
-                const skippedTests = differenceBy(
-                    testPlanReport.runnableTests,
-                    testPlanReport.finalizedTestResults,
-                    testOrTestResult =>
-                        testOrTestResult.test?.id ?? testOrTestResult.id
-                );
                 const overallMetrics = getMetrics({ testPlanReport });
 
                 const { at, browser } = testPlanReport;
@@ -124,17 +117,6 @@ const SummarizeTestPlanVersion = ({ testPlanVersion, testPlanReports }) => {
                                 View Complete Results
                             </Button>
                         </LinkContainer>
-                        {skippedTests.length ? (
-                            <Link
-                                to={
-                                    `/report/${testPlanVersion.id}` +
-                                    `/targets/${testPlanReport.id}` +
-                                    `#skipped-tests`
-                                }
-                            >
-                                {skippedTests.length} Tests Were Skipped
-                            </Link>
-                        ) : null}
                         <Table
                             className="mt-3"
                             bordered
diff --git a/client/components/TestQueueRow/index.jsx b/client/components/TestQueueRow/index.jsx
index b8df4534b..9e3b44790 100644
--- a/client/components/TestQueueRow/index.jsx
+++ b/client/components/TestQueueRow/index.jsx
@@ -414,6 +414,11 @@ const TestQueueRow = ({
             'completed'
         ].join('-');
 
+    const completedAllTests = testPlanReport.draftTestPlanRuns.every(
+        testPlanRun =>
+            testPlanRun.testResultsLength === testPlanReport.runnableTestsLength
+    );
+
     return (
         <LoadingStatus message={loadingMessage}>
             <tr className="test-queue-run-row">
@@ -494,7 +499,8 @@ const TestQueueRow = ({
                             !testPlanReport.conflictsLength &&
                             testPlanReport.draftTestPlanRuns.length &&
                             testPlanReport.draftTestPlanRuns[0]
-                                .testResultsLength ? (
+                                .testResultsLength &&
+                            completedAllTests ? (
                                 <>
                                     <Button
                                         ref={updateTestPlanStatusButtonRef}
diff --git a/client/components/TestRun/TestRun.css b/client/components/TestRun/TestRun.css
index ddaf9e32b..6a3b3f001 100644
--- a/client/components/TestRun/TestRun.css
+++ b/client/components/TestRun/TestRun.css
@@ -135,7 +135,7 @@ button.test-navigator-toggle:focus {
     left: 4px;
 }
 
-.test-name-wrapper.skipped .progress-indicator {
+.test-name-wrapper .progress-indicator {
     background: White;
     border: 2px dashed black;
 }
diff --git a/server/migrations/20231031162340-verifyNoSkippedTests.js b/server/migrations/20231031162340-verifyNoSkippedTests.js
new file mode 100644
index 000000000..c186aaa84
--- /dev/null
+++ b/server/migrations/20231031162340-verifyNoSkippedTests.js
@@ -0,0 +1,63 @@
+'use strict';
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+    async up(queryInterface /* , Sequelize */) {
+        return queryInterface.sequelize.transaction(async transaction => {
+            const [rows] = await queryInterface.sequelize.query(
+                `
+                    SELECT
+                        "TestPlanReport".id,
+                        "TestPlanReport"."atId",
+                        "TestPlanRun"."testResults",
+                        "TestPlanVersion".tests
+                    FROM
+                        "TestPlanReport"
+                        INNER JOIN "TestPlanVersion" ON "TestPlanVersion".id = "TestPlanReport"."testPlanVersionId"
+                        INNER JOIN "TestPlanRun" ON "TestPlanReport".id = "TestPlanRun"."testPlanReportId"
+                    WHERE
+                        "TestPlanReport"."markedFinalAt" IS NOT NULL
+                `,
+                { transaction }
+            );
+
+            const dataById = {};
+            rows.forEach(({ id, atId, testResults, tests }) => {
+                if (dataById[id]) {
+                    dataById[id].runLengths.push(testResults.length);
+                } else {
+                    const runnableTestsLength = tests.filter(test =>
+                        test.atIds.includes(atId)
+                    ).length;
+                    dataById[id] = {
+                        id,
+                        runnableTestsLength,
+                        runLengths: [testResults.length]
+                    };
+                }
+            });
+
+            for (const data of Object.values(dataById)) {
+                const isComplete = data.runLengths.every(
+                    runLength => runLength === data.runnableTestsLength
+                );
+                if (!isComplete) {
+                    console.info(`Found incomplete report ${data.id}`); // eslint-disable-line
+                    // Since final test plan reports can
+                    // no longer have skipped tests, we should
+                    // move them to the test queue where incomplete
+                    // runs are still supported.
+                    await queryInterface.sequelize.query(
+                        `
+                            UPDATE "TestPlanReport" SET "markedFinalAt" = NULL WHERE id = ?
+                        `,
+                        {
+                            replacements: [data.id],
+                            transaction
+                        }
+                    );
+                }
+            }
+        });
+    }
+};
diff --git a/server/resolvers/TestPlanReport/finalizedTestResultsResolver.js b/server/resolvers/TestPlanReport/finalizedTestResultsResolver.js
index 89cb7ccdb..fae66da6c 100644
--- a/server/resolvers/TestPlanReport/finalizedTestResultsResolver.js
+++ b/server/resolvers/TestPlanReport/finalizedTestResultsResolver.js
@@ -1,31 +1,20 @@
 const testResultsResolver = require('../TestPlanRun/testResultsResolver');
-const deepCustomMerge = require('../../util/deepCustomMerge');
 
-/**
- * Completed test results sourced from all the report's runs. The runs must be
- * merged because each run might have skipped different tests.
- */
 const finalizedTestResultsResolver = async (testPlanReport, _, context) => {
     if (!testPlanReport.testPlanRuns.length) {
         return null;
     }
 
-    let merged = [];
+    // Since conflicts are now resolved, all testPlanRuns are interchangeable.
+    const testPlanRun = testPlanReport.testPlanRuns[0];
 
-    for (let i = 0; i < testPlanReport.testPlanRuns.length; i += 1) {
-        merged = deepCustomMerge(
-            merged,
-            testPlanReport.testPlanRuns[i].testResults.filter(
-                testResult => !!testResult.completedAt
-            ),
-            {
-                identifyArrayItem: item =>
-                    item.testId ?? item.scenarioId ?? item.assertionId
-            }
-        );
-    }
     return testResultsResolver(
-        { testPlanReport, testResults: merged },
+        {
+            testPlanReport,
+            testResults: testPlanRun.testResults.filter(
+                testResult => !!testResult.completedAt
+            )
+        },
         null,
         context
     );
diff --git a/server/resolvers/TestPlanReportOperations/markAsFinalResolver.js b/server/resolvers/TestPlanReportOperations/markAsFinalResolver.js
index 56ba497e3..c552179c1 100644
--- a/server/resolvers/TestPlanReportOperations/markAsFinalResolver.js
+++ b/server/resolvers/TestPlanReportOperations/markAsFinalResolver.js
@@ -3,7 +3,7 @@ const {
     getTestPlanReportById,
     updateTestPlanReport
 } = require('../../models/services/TestPlanReportService');
-const finalizedTestResultsResolver = require('../TestPlanReport/finalizedTestResultsResolver');
+const runnableTestsResolver = require('../TestPlanReport/runnableTestsResolver');
 const populateData = require('../../services/PopulatedData/populateData');
 const conflictsResolver = require('../TestPlanReport/conflictsResolver');
 
@@ -27,15 +27,17 @@ const markAsFinalResolver = async (
         );
     }
 
-    const finalizedTestResults = await finalizedTestResultsResolver(
-        testPlanReport,
-        null,
-        context
+    const runnableTests = runnableTestsResolver(testPlanReport);
+
+    const hasIncompleteTestRuns = testPlanReport.testPlanRuns.find(
+        testPlanRun => {
+            return testPlanRun.testResults.length !== runnableTests.length;
+        }
     );
-    if (!finalizedTestResults || !finalizedTestResults.length) {
+
+    if (hasIncompleteTestRuns) {
         throw new Error(
-            'Cannot mark test plan report as final because there are no ' +
-                'completed test results'
+            'Cannot mark test plan report as final because not all testers have completed their test runs.'
         );
     }
 
diff --git a/server/resolvers/testPlanReportsResolver.js b/server/resolvers/testPlanReportsResolver.js
index fcbcae476..43090c157 100644
--- a/server/resolvers/testPlanReportsResolver.js
+++ b/server/resolvers/testPlanReportsResolver.js
@@ -30,13 +30,6 @@ const testPlanReportsResolver = async (
         attributes: testPlanReportAttributes
     } = retrieveAttributes('testPlanReport', TEST_PLAN_REPORT_ATTRIBUTES, info);
 
-    const { attributes: testPlanRunAttributes } = retrieveAttributes(
-        'draftTestPlanRuns',
-        TEST_PLAN_RUN_ATTRIBUTES,
-        info,
-        true
-    );
-
     const { attributes: testPlanVersionAttributes } = retrieveAttributes(
         'testPlanVersion',
         TEST_PLAN_VERSION_ATTRIBUTES,
@@ -64,7 +57,7 @@ const testPlanReportsResolver = async (
         null,
         where,
         testPlanReportAttributes,
-        testPlanRunAttributes,
+        TEST_PLAN_RUN_ATTRIBUTES,
         testPlanVersionAttributes,
         null,
         null,
diff --git a/server/scripts/populate-test-data/index.js b/server/scripts/populate-test-data/index.js
index 9c285042e..2fbb791d3 100644
--- a/server/scripts/populate-test-data/index.js
+++ b/server/scripts/populate-test-data/index.js
@@ -64,7 +64,7 @@ const populateTestDatabase = async () => {
 
     await populateFakeTestResults(4, [
         'completeAndPassing',
-        null,
+        'completeAndPassing',
         'completeAndFailingDueToIncorrectAssertions',
         'completeAndFailingDueToNoOutputAssertions',
         'completeAndFailingDueToUnexpectedBehaviors',
@@ -78,7 +78,7 @@ const populateTestDatabase = async () => {
 
     await populateFakeTestResults(5, [
         'completeAndPassing',
-        null,
+        'completeAndPassing',
         'completeAndFailingDueToIncorrectAssertions',
         'completeAndPassing',
         'completeAndFailingDueToNoOutputAssertions',
@@ -89,7 +89,7 @@ const populateTestDatabase = async () => {
 
     await populateFakeTestResults(6, [
         'completeAndPassing',
-        null,
+        'completeAndPassing',
         'completeAndFailingDueToIncorrectAssertions',
         'completeAndPassing',
         'completeAndFailingDueToIncorrectAssertions',
diff --git a/server/scripts/populate-test-data/populateFakeTestResults.js b/server/scripts/populate-test-data/populateFakeTestResults.js
index 0278cf0c0..a0c0e6d43 100644
--- a/server/scripts/populate-test-data/populateFakeTestResults.js
+++ b/server/scripts/populate-test-data/populateFakeTestResults.js
@@ -6,7 +6,28 @@ const {
 const { query, mutate } = require('../../tests/util/graphql-test-utilities');
 
 const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
+    const {
+        populateData: { testPlanReport }
+    } = await query(gql`
+        query {
+            populateData(locationOfData: { testPlanRunId: ${testPlanRunId} }) {
+                testPlanReport {
+                    at {
+                        id
+                    }
+                    browser {
+                        id
+                    }
+                    runnableTests {
+                        id
+                    }
+                }
+            }
+        }
+    `);
+
     let index = 0;
+
     for (const fakeTestResultType of fakeTestResultTypes) {
         // eslint-disable-next-line no-console
         console.info(
@@ -21,6 +42,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'completeAndPassing':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'passing',
@@ -29,6 +51,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'completeAndFailingDueToIncorrectAssertions':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToIncorrectAssertions',
@@ -37,6 +60,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'completeAndFailingDueToNoOutputAssertions':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToNoOutputAssertions',
@@ -45,6 +69,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'completeAndFailingDueToUnexpectedBehaviors':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToUnexpectedBehaviors',
@@ -53,6 +78,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'completeAndFailingDueToMultiple':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToMultiple',
@@ -61,6 +87,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'incompleteAndEmpty':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'empty',
@@ -69,6 +96,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'incompleteAndPassing':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'passing',
@@ -77,6 +105,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'incompleteAndFailingDueToIncorrectAssertions':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToIncorrectAssertions',
@@ -85,6 +114,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'incompleteAndFailingDueToNoOutputAssertions':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToNoOutputAssertions',
@@ -93,6 +123,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'incompleteAndFailingDueToUnexpectedBehaviors':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToUnexpectedBehaviors',
@@ -101,6 +132,7 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
                 break;
             case 'incompleteAndFailingDueToMultiple':
                 await getFake({
+                    testPlanReport,
                     testPlanRunId,
                     index,
                     fakeTestResultType: 'failingDueToMultiple',
@@ -115,34 +147,31 @@ const populateFakeTestResults = async (testPlanRunId, fakeTestResultTypes) => {
 
         index += 1;
     }
+
+    // The following makes it so that fake tests are made for
+    // all possible tests that can be completed for any given
+    // test plan. Therefore, there will be no missing or skipped
+    // fake tests in the database.
+    if (testPlanReport.runnableTests.length !== fakeTestResultTypes.length) {
+        for (let i = index; i < testPlanReport.runnableTests.length; i += 1) {
+            await getFake({
+                testPlanReport,
+                testPlanRunId,
+                index: i,
+                fakeTestResultType: 'passing',
+                submit: true
+            });
+        }
+    }
 };
 
 const getFake = async ({
+    testPlanReport,
     testPlanRunId,
     index,
     fakeTestResultType,
     submit
 }) => {
-    const {
-        populateData: { testPlanReport }
-    } = await query(gql`
-        query {
-            populateData(locationOfData: { testPlanRunId: ${testPlanRunId} }) {
-                testPlanReport {
-                    at {
-                        id
-                    }
-                    browser {
-                        id
-                    }
-                    runnableTests {
-                        id
-                    }
-                }
-            }
-        }
-    `);
-
     const testId = testPlanReport.runnableTests[index].id;
 
     const atVersion = await getAtVersionByQuery(
diff --git a/server/tests/integration/graphql.test.js b/server/tests/integration/graphql.test.js
index 3a7a3f8db..08414505b 100644
--- a/server/tests/integration/graphql.test.js
+++ b/server/tests/integration/graphql.test.js
@@ -595,13 +595,13 @@ describe('graphql', () => {
                                 }
                             }
                         }
-                        markReportAsFinal: testPlanReport(id: 2) {
+                        markReportAsFinal: testPlanReport(id: 8) {
                             __typename
                             markAsFinal {
                                 locationOfData
                             }
                         }
-                        unmarkReportAsFinal: testPlanReport(id: 2) {
+                        unmarkReportAsFinal: testPlanReport(id: 8) {
                             __typename
                             unmarkAsFinal {
                                 locationOfData