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

@@ -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;
 }

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>
     );
 };

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

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}

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;
 }

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
+                        }
+                    );
+                }
+            }
+        });
+    }
+};

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
     );

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.'
         );
     }
 

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,

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',