Passed/failed for assertion results in TestRenderer and migrate to pass/fail for report comparison algorithms #847

stalgiag · 2023-11-21T00:40:01Z

This PR does the following:

Changes input elements for test runs to a fieldset with checkbox inputs.
Updates test rendered component to use correct input change callbacks on updated assertion data structure
Updates result comparison algorithm so that it no longer takes failedReason into account when calculating conflicts

For reviewers, the client/resources changes are not relevant to this PR and have been reviewed elsewhere.

stalgiag · 2023-11-21T00:42:59Z

client/components/TestRenderer/AssertionsFieldset/index.jsx

+
+const AssertionsFieldset = ({ assertions, commandIndex, assertionsHeader }) => {
+    // Handle case where build process didn't include assertionResponseQuestion
+    const normalizedHeader = useMemo(() => {


The value from support.json wasn't being correctly included with test commands in assertionsHeader.descriptionHeader. I am not sure but my sense is that this isn't from changes in this PR but instead comes from the state of the work that this PR is built upon. If this isn't the case, let me know. Either way this calculation handles the issue and it can become a tiny tech debt task after all of the v2 changes have landed.

The renderer in this app has to be built on top of the individual collected json files. Architecturally, that means the .collected.json for any of these tests have to include the relevant data. Looking into this, it's clear there is some extending that can happen here to support that. The same issue is present when viewing a example collected test.

@stalgiag I've submitted w3c/aria-at#1005 as a potential fix. Let me know your thoughts.

There's other potential tech debt I've noticed, that may come up during future v2 format support work, that I think we can handle as they arise.

Looking again at how this is used, I'm also not against just explicitly depending on the support.json in this use case. It should always be up to date because of the import script.

Let's leave as-is for now and I'll check out #1005 for sweeping this up later. Definitely would be nice to avoid the weirdness of looking for the undefined value here.

Circling back around - It doesn't seem that collectedText.supportJSON is coming through with the changes in #1005. I can look more into this next week if we think it is important to handle now.

It doesn't seem that collectedText.supportJSON is coming through with the changes

The supportJson attribute would be added in the object called by the following line as well:

await testRunIO.setInputsFromCollectedTestAsync(renderableContent)

if we think it is important to handle now.

I don't right now. Still fully onboard with just calling it from the available support.json.

howard-e

These changes look good to me. Although I imagine in the future, we'll want to clean up references to failedReason.

Also I think these references to failedReason being updated in the harness' state may need to be updated to be safe? I imagine it's something like the following based on your other changes:

                 if (assertionResult.passed)
                     commands[i].assertions[j].result = 'pass';
-                else if (assertionResult.failedReason === 'NO_OUTPUT')
-                    commands[i].assertions[j].result = 'failMissing';
-                else if (assertionResult.failedReason === 'INCORRECT_OUTPUT')
-                    commands[i].assertions[j].result = 'failIncorrect';
-                else commands[i].assertions[j].result = 'notSet';
+                else commands[i].assertions[j].result = 'fail';

server/resolvers/TestResultOperations/saveTestResultCommon.js

howard-e · 2023-11-21T20:29:07Z

client/components/TestRenderer/AssertionsFieldset/index.jsx

+
+const AssertionsFieldset = ({ assertions, commandIndex, assertionsHeader }) => {
+    // Handle case where build process didn't include assertionResponseQuestion
+    const normalizedHeader = useMemo(() => {


Looking again at how this is used, I'm also not against just explicitly depending on the support.json in this use case. It should always be up to date because of the import script.

stalgiag · 2023-11-22T22:35:25Z

These changes look good to me. Although I imagine in the future, we'll want to clean up references to failedReason.

Also I think these references to failedReason being updated in the harness' state may need to be updated to be safe? I imagine it's something like the following based on your other changes:

                 if (assertionResult.passed)
                     commands[i].assertions[j].result = 'pass';
-                else if (assertionResult.failedReason === 'NO_OUTPUT')
-                    commands[i].assertions[j].result = 'failMissing';
-                else if (assertionResult.failedReason === 'INCORRECT_OUTPUT')
-                    commands[i].assertions[j].result = 'failIncorrect';
-                else commands[i].assertions[j].result = 'notSet';
+                else commands[i].assertions[j].result = 'fail';

Thanks for this! I forgot that this value is not just excluded from v2, it is being deprecated from use across all test versions. This allowed me to more destructively remove the references. I only left them in two locations:

GraphQL schema, just as documentation to explain the DB
populateTestData functions. Perhaps these will need to be removed. I was unsure of how much we want to rewrite our base local testing state.

This also means that there is some additional clean up that can happen upstream in the resources files in aria-at. I'll handle that next week.

howard-e · 2023-11-27T19:34:38Z

This also means that there is some additional clean up that can happen upstream in the resources files in aria-at. I'll handle that next week.

Sounds like a plan, thanks for being on top of that!

…ertion results

Co-authored-by: Howard Edwards <[email protected]>

stalgiag commented Nov 21, 2023

View reviewed changes

stalgiag requested a review from howard-e November 21, 2023 00:44

howard-e mentioned this pull request Nov 21, 2023

Include content from support.json properties when using loadCollectedTestAsync w3c/aria-at#1005

Closed

howard-e reviewed Nov 21, 2023

View reviewed changes

stalgiag requested a review from howard-e November 27, 2023 16:50

howard-e force-pushed the v2-test-format-testrun branch from cd1cdc9 to aaa628c Compare November 27, 2023 19:00

howard-e approved these changes Nov 27, 2023

View reviewed changes

howard-e force-pushed the pass-fail-test-run branch from 7212f02 to 5caad44 Compare November 27, 2023 20:58

stalgiag and others added 8 commits November 27, 2023 16:05

Passed/failed for assertion results in TestRenderer

d93a024

Update test plan report conflict calculation to support pass/fail ass…

c836bf5

…ertion results

Correct assertion legend, margin right for checkbox

6792426

Update inline snapshot for test queue

842344f

Remove failedReason

9446574

Update server/resolvers/TestResultOperations/saveTestResultCommon.js

d9e75cb

Co-authored-by: Howard Edwards <[email protected]>

Fix linting issue in saveTestResultCommon

ebe846a

Undo change to checkAssertionResult

e9f4388

howard-e force-pushed the pass-fail-test-run branch from 5caad44 to e9f4388 Compare November 27, 2023 21:06

howard-e mentioned this pull request Nov 29, 2023

Update Reports for V2 Test Format #845

Merged

Base automatically changed from v2-test-format-testrun to support-v2-test-format November 29, 2023 17:25

This was referenced Nov 29, 2023

Update Test Runner for V2 Test Format #843

Merged

Support v2 Test Format in import script and page changes #840

Merged

howard-e merged commit 495543d into support-v2-test-format Nov 29, 2023
2 checks passed

howard-e deleted the pass-fail-test-run branch November 29, 2023 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passed/failed for assertion results in TestRenderer and migrate to pass/fail for report comparison algorithms #847

Passed/failed for assertion results in TestRenderer and migrate to pass/fail for report comparison algorithms #847

stalgiag commented Nov 21, 2023

stalgiag Nov 21, 2023 •

edited

Loading

howard-e Nov 21, 2023 •

edited

Loading

howard-e Nov 21, 2023 •

edited

Loading

howard-e Nov 21, 2023

stalgiag Nov 22, 2023

stalgiag Nov 22, 2023

howard-e Nov 27, 2023

howard-e left a comment

howard-e Nov 21, 2023

stalgiag commented Nov 22, 2023 •

edited

Loading

howard-e commented Nov 27, 2023

Passed/failed for assertion results in TestRenderer and migrate to pass/fail for report comparison algorithms #847

Passed/failed for assertion results in TestRenderer and migrate to pass/fail for report comparison algorithms #847

Conversation

stalgiag commented Nov 21, 2023

stalgiag Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

howard-e Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

howard-e Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

howard-e Nov 21, 2023

Choose a reason for hiding this comment

stalgiag Nov 22, 2023

Choose a reason for hiding this comment

stalgiag Nov 22, 2023

Choose a reason for hiding this comment

howard-e Nov 27, 2023

Choose a reason for hiding this comment

howard-e left a comment

Choose a reason for hiding this comment

howard-e Nov 21, 2023

Choose a reason for hiding this comment

stalgiag commented Nov 22, 2023 • edited Loading

howard-e commented Nov 27, 2023

stalgiag Nov 21, 2023 •

edited

Loading

howard-e Nov 21, 2023 •

edited

Loading

howard-e Nov 21, 2023 •

edited

Loading

stalgiag commented Nov 22, 2023 •

edited

Loading