Remove all instances of REQUIRED and OPTIONAL #938

Paul-Clue · 2024-02-27T15:16:59Z

see issue #868

This PR removes all instances of REQUIRED and OPTIONAL. These changes already exist in presentational components. This PR updates the references of those priority names in the code itself to lessen any confusion in the future.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1206594245402143

howard-e

@Paul-Clue thanks for taking this first step to update the repository to be consistent with the new language!

I think the key thing that may be missing here though is that a database migration is needed to update the older TestPlanVersion.tests[].assertions[].priority which still use REQUIRED and OPTIONAL as well.

howard-e · 2024-02-27T20:31:17Z

client/resources/aria-at-test-io-format.mjs

@@ -1455,7 +1455,7 @@ export class TestRunInputOutput {
        output: command.atOutput.value,
        assertionResults: command.assertions.map(assertion => ({
          assertion: {
-            priority: assertion.priority === 1 ? 'REQUIRED' : 'OPTIONAL',
+            priority: assertion.priority === 1 ? 'MUST' : 'SHOULD',


Changes that happen to files inside client/resources/* should also have an equivalent PR making that change in w3c/aria-at (until we get to optimizing the import process to eliminate this need)

howard-e · 2024-02-27T21:03:51Z

client/resources/aria-at-test-io-format.mjs

@@ -1642,7 +1642,7 @@ const UnexpectedBehaviorImpactMap = createEnumMap({
 /**
 * Command result derived from priority 1 and 2 assertions.
 *
- * Support is "FAILING" is priority 1 assertions fail. Support is "ALL REQUIRED"
+ * Support is "FAILING" is priority 1 assertions fail. Support is "ALL MUST"


I don't think this has to be changed

Agreed. If you do change it, make sure to look at server/util/getMetrics.js which still uses the term "All Required." Probably better to just leave it as-is.

howard-e · 2024-02-27T21:04:15Z

client/resources/aria-at-test-io-format.mjs

@@ -1653,7 +1653,7 @@ const UnexpectedBehaviorImpactMap = createEnumMap({
 const CommandSupportJSONMap = createEnumMap({
  FULL: 'FULL',
  FAILING: 'FAILING',
-  ALL_REQUIRED: 'ALL REQUIRED',
+  ALL_REQUIRED: 'ALL MUST',


Same with this, but if it were to change, the enum name may as well be changed too

alflennik

I concur with Howard that the main missing thing is the migration. Once that's done I'll run the changes and see if I can find any issues. As for the code, it looks good, not much to add to Howard's review.

alflennik · 2024-02-28T22:25:43Z

server/util/convertAssertionPriority.js

@@ -8,14 +8,13 @@
 * @returns {null|string}


The TODO can be removed now.

alflennik · 2024-02-28T22:27:08Z

client/resources/aria-at-test-io-format.mjs

@@ -1642,7 +1642,7 @@ const UnexpectedBehaviorImpactMap = createEnumMap({
 /**
 * Command result derived from priority 1 and 2 assertions.
 *
- * Support is "FAILING" is priority 1 assertions fail. Support is "ALL REQUIRED"
+ * Support is "FAILING" is priority 1 assertions fail. Support is "ALL MUST"


Agreed. If you do change it, make sure to look at server/util/getMetrics.js which still uses the term "All Required." Probably better to just leave it as-is.

howard-e

@Paul-Clue looks good! There's an issue I found with the migration when testing against the current prod data and have suggested an inline fix.

Also, you can remove REQUIRED and OPTIONAL for the AssertionPriority enum in graphql-schema.js

howard-e · 2024-03-14T22:16:57Z

server/migrations/20240311192910-removeRequiredAndOptionalPriority.js

+                            EXISTS (
+                              SELECT
+                                1
+                              FROM
+                                jsonb_array_elements(tests) AS test,
+                                jsonb_array_elements(test -> 'assertions') AS assertion
+                              WHERE
+                                assertion ->> 'priority' = 'REQUIRED'
+                                OR assertion ->> 'priority' = 'OPTIONAL'
+                            )`,


Suggested change

EXISTS (

SELECT

1

FROM

jsonb_array_elements(tests) AS test,

jsonb_array_elements(test -> 'assertions') AS assertion

WHERE

assertion ->> 'priority' = 'REQUIRED'

OR assertion ->> 'priority' = 'OPTIONAL'

)`,

id = ?`,

The sequelize replacements option is expecting an id replacement which wasn't being used so I assume this must have been the thought. It's much safer to also make this update using the queried id involved in the loop. Using the previous condition could have unexpected behaviors.

howard-e · 2024-03-14T22:29:03Z

shared/convertAssertionPriority.js

 *
 * @param {number|string} priority
- * @returns {null|string}
+ * @returns {null|string}


Suggested change

* @returns {null|string}

* @returns {null|string}

alflennik · 2024-03-18T17:07:54Z

@Paul-Clue looks like the tests are failing, any idea about that?

howard-e · 2024-03-18T20:47:27Z

shared/getMetrics.js

@@ -191,7 +191,7 @@ const getMetrics = ({
        assertionsCount: requiredAssertionsCount,
        assertionsPassedCount: requiredAssertionsPassedCount,
        assertionsFailedCount: requiredAssertionsFailedCount
-    } = calculateAssertionPriorityCounts(result, 'REQUIRED');
+    } = calculateAssertionPriorityCounts(result, 'MUST');


So with this change, because

aria-at-app/shared/getMetrics.js

Line 46 in 3808ed7

`${priority.toLowerCase()}AssertionResults` in scenarioResult;

uses the priority string in combination with what has been named through the client's various queries.js files, it will incorrectly display total values anywhere TestPlanResultsTable is getting used.

ie. queries.js provides requiredAssertionResults but this change to getMetrics is expecting mustAssertionResults.

The following screenshot also shows that for each command, there are 2+ assertions but the command's assertion level status only shows 2 passed, which would then also affect the total calculation for the entire test:

2 things I think could happen here:

Revert the change to getMetrics until updating each of the named (required|optional)AssertionResults instances. But this may cause some confusion until then.

Update the named instances of (required|optional)AssertionResults as appropriate which means renaming the value being exported from getMetrics and also adding a migration to update metrics across all the TestPlanReports

Hey @howard-e all of the changes have been made and a migration created.

Great! Thanks for going the route of no. 2 as well. Definitely makes things cleaner for the future!

alflennik · 2024-03-19T22:06:01Z

shared/getMetrics.js

@@ -219,11 +219,11 @@ const getMetrics = ({
        testPlanReport?.runnableTests?.length || countTests({ ...result });
    const testsFailedCount = testsCount - testsPassedCount;

-    const requiredFormatted = `${requiredAssertionsPassedCount} of ${requiredAssertionsCount} passed`;
+    const requiredFormatted = `${mustAssertionsPassedCount} of ${mustAssertionsCount} passed`;
    const optionalFormatted =


I think requiredFormatted needs to be mustFormatted and optionalFormatted needs to be shouldFormatted, right?

alflennik · 2024-03-19T22:09:09Z

@Paul-Clue fantastic work, I just have one comment. I verified everything else is working and once you address that I'll approve.

alflennik

Thanks for your considerable persistence chasing down all the details needed to get this task done! 😜

howard-e

Looks good to go after fixing my latest inline comment

howard-e · 2024-03-20T14:08:18Z

client/components/CandidateReview/CandidateTestPlanRun/queries.js

-                    optionalAssertionResults: assertionResults(
-                        priority: OPTIONAL
-                    ) {
+                    mustAssertionResults: assertionResults(priority: SHOULD) {


Suggested change

mustAssertionResults: assertionResults(priority: SHOULD) {

shouldAssertionResults: assertionResults(priority: SHOULD) {

Got it! Thanks @howard-e!

howard-e

Looks good! Thanks for the continued changes to perfect and support this change @Paul-Clue!

ccanash · 2024-03-21T23:11:05Z

See issue #926

Remove all of required and optional

94bd4e8

Paul-Clue requested review from howard-e and alflennik February 27, 2024 15:17

howard-e requested changes Feb 27, 2024

View reviewed changes

alflennik requested changes Feb 28, 2024

View reviewed changes

Revert some changes to aria-at-test-io

e9d595e

howard-e mentioned this pull request Mar 4, 2024

Address single page view format issues #944

Merged

Paul-Clue added 3 commits March 11, 2024 15:56

Start creating migration

0af140d

Add migration to change assertion.priority

bcd55e7

Fix merge conflicts

27f6f92

howard-e requested changes Mar 14, 2024

View reviewed changes

Fix migration and address other changes per code review

4c36d25

Fix query

3808ed7

howard-e requested changes Mar 18, 2024

View reviewed changes

Paul-Clue added 2 commits March 19, 2024 14:24

Remove all references of required

0467136

Add migration for getMetrics

05aff0b

alflennik reviewed Mar 19, 2024

View reviewed changes

Paul-Clue added 3 commits March 19, 2024 17:14

Replace more instances of required and optional

d47c010

Remove requiredFormatted and optionalFormatted

4f2a546

Remove required and optional referrences

3a1377e

alflennik approved these changes Mar 19, 2024

View reviewed changes

howard-e reviewed Mar 20, 2024

View reviewed changes

Paul-Clue added 4 commits March 20, 2024 09:51

Change must to should

86c397e

<AtAndBrowserDetailsModal

ab37e2f

Fix failing test

7c0dcb1

Fix variable name

aba198b

howard-e approved these changes Mar 20, 2024

View reviewed changes

howard-e merged commit 21a1e1c into main Mar 20, 2024
2 checks passed

howard-e deleted the replace-required-and-must branch March 20, 2024 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all instances of REQUIRED and OPTIONAL #938

Remove all instances of REQUIRED and OPTIONAL #938

Paul-Clue commented Feb 27, 2024 •

edited by ccanash

Loading

howard-e left a comment

howard-e Feb 27, 2024

howard-e Feb 27, 2024

alflennik Feb 28, 2024

howard-e Feb 27, 2024

alflennik left a comment

alflennik Feb 28, 2024

Paul-Clue Mar 11, 2024

alflennik Feb 28, 2024

howard-e left a comment

howard-e Mar 14, 2024 •

edited

Loading

howard-e Mar 14, 2024

alflennik commented Mar 18, 2024

howard-e Mar 18, 2024 •

edited

Loading

Paul-Clue Mar 19, 2024

howard-e Mar 20, 2024 •

edited

Loading

alflennik Mar 19, 2024

Paul-Clue Mar 19, 2024

alflennik commented Mar 19, 2024

alflennik left a comment

howard-e left a comment •

edited

Loading

howard-e Mar 20, 2024

Paul-Clue Mar 20, 2024 •

edited

Loading

howard-e left a comment

ccanash commented Mar 21, 2024

	mustAssertionResults: assertionResults(priority: SHOULD) {
	shouldAssertionResults: assertionResults(priority: SHOULD) {

Remove all instances of REQUIRED and OPTIONAL #938

Remove all instances of REQUIRED and OPTIONAL #938

Conversation

Paul-Clue commented Feb 27, 2024 • edited by ccanash Loading

howard-e 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

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

howard-e left a comment

Choose a reason for hiding this comment

howard-e Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alflennik commented Mar 18, 2024

howard-e Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howard-e Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alflennik commented Mar 19, 2024

alflennik left a comment

Choose a reason for hiding this comment

howard-e left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Clue Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

howard-e left a comment

Choose a reason for hiding this comment

ccanash commented Mar 21, 2024

Paul-Clue commented Feb 27, 2024 •

edited by ccanash

Loading

howard-e Mar 14, 2024 •

edited

Loading

howard-e Mar 18, 2024 •

edited

Loading

howard-e Mar 20, 2024 •

edited

Loading

howard-e left a comment •

edited

Loading

Paul-Clue Mar 20, 2024 •

edited

Loading