Update Test Runner for V2 Test Format #843

howard-e · 2023-11-15T22:56:37Z

See #743. Based on #840.

This PR also updates <InstructionsRenderer> which was being used on the Candidate Test Plan Run page.

Dependent PRs to be merged

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

client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx

alflennik

Generally the code looks great! I did leave one pretty lengthy comment as you can see, about the location where we are persisting the testFormatVersion variable. I'm hypothesizing that it would be a large scale change requiring a lot of effort (I might be wrong) so that's why I went into more detail about why I think it's worth it.

Note I haven't yet been able to test this locally but I'm working on it.

alflennik · 2023-11-21T17:11:31Z

client/components/TestRenderer/index.jsx

@@ -216,6 +212,7 @@ const TestRenderer = ({
    at,
    testResult = {},
    testPageUrl,
+    testFormatVersion = 1,


I think my preference would be to make the testFormatVersion a required field, I feel like defaulting to v1 kind of sends a message that version 1 is privileged in some way or preferable, when that's not the case.

Done in c29eef0

alflennik · 2023-11-21T21:57:33Z

client/package.json

@@ -43,6 +43,7 @@
    "react-bootstrap": "^2.7.0",
    "react-dom": "^18.2.0",
    "react-helmet": "^6.0.0",
+    "react-html-parser": "^2.0.2",


Cool package 👍

alflennik · 2023-11-21T21:59:23Z

client/components/TestRenderer/utils.js

+const NumberedList = styled.ol`
+    > li {
+    }
+`;


Is this needed?

Nope! Got caught copying and pasting 🙂

Done in eab5ec8

alflennik · 2023-11-21T22:42:51Z

client/components/TestRun/index.jsx

@@ -1011,6 +1011,10 @@ const TestRun = () => {
                                              )
                                    }
                                    testPageUrl={testPlanVersion.testPageUrl}
+                                    testFormatVersion={
+                                        testPlanVersion.metadata
+                                            .testFormatVersion


Big ask here: can the testFormatVersion be included inside the renderableContent?

It's a pretty sizable change, but here's my reasoning about why it might be worth it.

When contributors come across this variable, called testFormatVersion, the implication is that there can be multiple ways tests are formatted. It's right there in the name. So what's going to happen is that they are going to doubt whether they can rely on the documented concepts like "scenarios" and "assertions" and "tests" and "testResults."

As you know, in reality all these concepts are completely unaffected by the testFormatVersion. But it seems like they could be. That's the disconnect that I want to try to mitigate.

If the testFormatVersion was only accessible from within the renderableContent, A.K.A. renderableContent.testFormatVersion, suddenly it becomes immediately apparent where the safe zone ends and where the danger zone starts. It becomes obvious that if you are attempting to extract some kind of data from the renderableContent, you should make sure to account for all the versions that the data can be formatted.

There reason it is a big ask is that this field is not currently included in the renderableContent and I think it would need to be added in a migration. But furthermore, to do it right, it seems like going forward the field would need to be added into the build process of the ARIA-AT repo for both v1 and v2 build processes.

That's a lot, definitely reach out if I can clarify anything I said above.

Answered by #840 (comment), which is also where that change would happen.

I don't quite follow why the testFormatVersion would live on individual tests since it will be consistent across the collection in the parent.

Also v2 of the testFormatVersion doesn't affect "scenarios", "assertions", "tests", or "testResults" but a future version could. But then again, I could be totally misunderstanding the suggestion here.

I decided to approve despite this comment, which might be motivated more from my misunderstanding than any genuine issue.

stalgiag · 2023-11-22T21:26:43Z

client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx

+        allInstructions = [
+            renderableContent.target.at.raw
+                .defaultConfigurationInstructionsHTML,
+            `${supportJson.testPlanStrings.openExampleInstruction} ${renderableContent.target.setupScript.scriptDescription}.`,
+            `${renderableContent.instructions.instructions} ${
+                supportJson.testPlanStrings.commandListPreface
+            }${
+                commandSettingSpecified
+                    ? ` ${supportJson.testPlanStrings.commandListSettingsPreface}`
+                    : ''
+            }`
+        ].map(e => unescape(e));


I have a hard time seeing where an array item starts and stops here. This is mainly due to the fact that our project uses the default printwidth in prettier but maybe this could benefit from a couple of intermediate variables just to make the array easier to read.

I have a hard time seeing where an array item starts and stops here. This is mainly due to the fact that our project uses the default printwidth in prettier

I'd love to increase this if we can find time to discuss on what's best. It has felt a bit much in other areas of the code I've across.

... benefit from a couple of intermediate variables just to make the array easier to read.

Done in da48e17 and 68e3eea

client/components/TestRenderer/utils.js

stalgiag · 2023-11-22T21:33:27Z

client/components/TestRenderer/utils.js

+        }
+    });
+    return settingsContent;
+};


Glad to have these in a separate file!

stalgiag

I am still learning how the different parts of this app work with aria-at but I believethat I was able to get this up and view the changes without any issues.

Code feedback is minimal. Mostly everything looks good. All suggestions are optional so I am approving.

Co-authored-by: Stalgia Grigg <[email protected]>

…nto v2-test-format-testrun

howard-e changed the title ~~V2 test format testrun~~ Update Test Runner for V2 Test Format Nov 15, 2023

howard-e force-pushed the v2-test-format-testrun branch 6 times, most recently from 2e34001 to 7114117 Compare November 16, 2023 22:39

howard-e marked this pull request as ready for review November 16, 2023 22:40

howard-e mentioned this pull request Nov 16, 2023

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

Merged

3 tasks

howard-e force-pushed the v2-test-format-testrun branch from 7114117 to c0ba18e Compare November 20, 2023 13:55

howard-e requested a review from stalgiag November 21, 2023 14:46

howard-e commented Nov 21, 2023

View reviewed changes

client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx Outdated Show resolved Hide resolved

howard-e requested a review from alflennik November 21, 2023 16:50

alflennik reviewed Nov 21, 2023

View reviewed changes

stalgiag approved these changes Nov 22, 2023

View reviewed changes

howard-e force-pushed the support-v2-test-format branch from 133ed89 to 8dd8119 Compare November 27, 2023 15:27

howard-e added 4 commits November 27, 2023 14:00

Updating Test Run page to support #743

25b9a39

Update Test Run page instructions for v2 test format

77cffe5

Add react-html-parser; update InstructionsRenderer

85b86d1

Use testPlanVersion.metadata.testFormatVersion

aaa628c

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

howard-e and others added 6 commits November 27, 2023 14:05

Stop defaulting testFormatVersion to 1

c29eef0

Stop using NumberedList

eab5ec8

Update client/components/TestRenderer/utils.js

54864ab

Co-authored-by: Stalgia Grigg <[email protected]>

Merge branch 'v2-test-format-testrun' of github.com:w3c/aria-at-app i…

4557fdc

…nto v2-test-format-testrun

Define instructions variables

da48e17

Consistency

68e3eea

alflennik approved these changes Nov 28, 2023

View reviewed changes

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

howard-e deleted the v2-test-format-testrun branch November 29, 2023 17:25

howard-e mentioned this pull request Feb 14, 2024

Consider updating automated code formatting tools' rules #924

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Test Runner for V2 Test Format #843

Update Test Runner for V2 Test Format #843

howard-e commented Nov 15, 2023 •

edited

Loading

alflennik left a comment

alflennik Nov 21, 2023

howard-e Nov 27, 2023

alflennik Nov 21, 2023

alflennik Nov 21, 2023

howard-e Nov 22, 2023

howard-e Nov 27, 2023

alflennik Nov 21, 2023

howard-e Nov 22, 2023

stalgiag Nov 22, 2023 •

edited

Loading

alflennik Nov 28, 2023

stalgiag Nov 22, 2023

howard-e Nov 27, 2023 •

edited

Loading

stalgiag Nov 22, 2023

stalgiag left a comment

Update Test Runner for V2 Test Format #843

Update Test Runner for V2 Test Format #843

Conversation

howard-e commented Nov 15, 2023 • edited Loading

alflennik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stalgiag Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howard-e Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stalgiag left a comment

Choose a reason for hiding this comment

howard-e commented Nov 15, 2023 •

edited

Loading

stalgiag Nov 22, 2023 •

edited

Loading

howard-e Nov 27, 2023 •

edited

Loading