Update commands.json file to commandsV1.js #936

Paul-Clue · 2024-02-26T19:42:27Z

see issue #868

This PR updates all of the commands.json files to commandV1.json in order to make it clear what version of commands are being used.

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

alflennik

Hey, great job Paul with this large-scale refactor (!!), I see you were able to figure out which command-related variables needed to change and which were irrelevant, which is absolutely not obvious and goes way beyond a find-replace. Big props for that.

I was able to import tests, run them, and see the test result summaries. Looks like the changed systems are all working.

I do have a few thoughts scattered around, nothing too big, but nice to see you coming through with a big PR like this.

alflennik · 2024-02-28T18:48:04Z

server/scripts/import-tests/index.js

@@ -327,7 +327,7 @@ const updateJsons = async () => {
            JSON.stringify(flattenObject(commandsV2Parsed), null, 4)
        );
    } catch (error) {
-        console.error('commands.json for v2 test format may not exist');
+        console.error('commandsV1.json for v2 test format may not exist');


This message doesn't really make sense!

Right, it could (should?) say commandsV2.json does not exist instead or some variation of that.

alflennik · 2024-02-28T18:50:51Z

server/resources/commandsV1.json

+        "id": "Z",
+        "text": "Z"
+    }
+]


Can you remove the commands.json file? It is unused and liable to cause confusion

alflennik · 2024-02-28T18:52:07Z

client/resources/at-commands.mjs

@@ -92,7 +92,7 @@ export class commandsAPI {
              command = keys[command];
              if (typeof command === 'undefined') {
                throw new Error(
-                  `Key instruction identifier "${c}" for AT "${assistiveTech.name}", mode "${mode}", task "${task}" is not an available identifier. Update your commands.json file to the correct identifier or add your identifier to resources/keys.mjs.`
+                  `Key instruction identifier "${c}" for AT "${assistiveTech.name}", mode "${mode}", task "${task}" is not an available identifier. Update your commandsV1.json file to the correct identifier or add your identifier to resources/keys.mjs.`


@howard-e correct me if I'm wrong but this is not definitively a v1 only issue, so this message should say "commandsV1.json or commandsV2.json" right?

Well the tests/commands.json is for controlling the defining keys used in v2 while tests/resources/keys.mjs is where the v1 key commands are defined (and then translated later on to some json).

So unless we're renaming the commands.json to commandsV2.json to make things clearer, this message can stay as is.

If that does get updated though, I think it would be preferred to have a context aware error message depending on the version, to avoid that general statement.

alflennik · 2024-02-28T18:52:25Z

client/resources/at-commands.mjs

@@ -111,7 +111,7 @@ export class commandsAPI {
            const commandKVs = this.findValuesByKeys([commandId]);
            if (!commandKVs.length) {
              throw new Error(
-                `Key instruction identifier "${commandId}" for AT "${assistiveTech.name}", mode "${mode}", task "${task}" is not an available identifier. Update your commands.json file to the correct identifier or add your identifier to tests/commands.json.`
+                `Key instruction identifier "${commandId}" for AT "${assistiveTech.name}", mode "${mode}", task "${task}" is not an available identifier. Update your commandsV1.json file to the correct identifier or add your identifier to tests/commandsV1.json.`


Same as above

alflennik · 2024-02-28T18:53:01Z

server/resolvers/helpers/retrieveCommands.js

@@ -1,4 +1,4 @@
-const commands = require('../../resources/commands.json');
+const commands = require('../../resources/commandsV1.json');


Can you call this commandsV1 to be symmetrical with commandsV2?

alflennik · 2024-02-28T18:55:05Z

client/resources/aria-at-harness.mjs

@@ -126,8 +126,8 @@ export async function loadCollectedTestAsync(testRoot, testFileName) {
  const collectedTestResponse = await fetch(`${testRoot}/${testFileName}`);
  const collectedTestJson = await collectedTestResponse.json();

-  // v2 commands.json
-  const commandsJsonResponse = await fetch('../commands.json');
+  // v2 commandsV1.json


I don't think this comment makes much sense. Is this a hint that there might be an issue with this particular change? Maybe you can explain to me what's happening here?

alflennik · 2024-02-28T18:56:54Z

client/resources/aria-at-harness.mjs

@@ -126,8 +126,8 @@ export async function loadCollectedTestAsync(testRoot, testFileName) {
  const collectedTestResponse = await fetch(`${testRoot}/${testFileName}`);


An important note is that any changes to the aria-at-harness.mjs and any other files in this folder will also need a corresponding PR over on the aria-at repo. So go ahead and open a PR over there as well, and feel free to reach out if I can clarify how that works.

alflennik · 2024-02-28T18:58:41Z

server/scripts/import-tests/index.js

@@ -309,14 +309,14 @@ const updateJsons = async () => {



Could you rename commands on line 306 to commandsV1Parsed to make it clear that this variable is equivalent to the variable below called commandsV2Parsed?

Paul-Clue · 2024-03-14T23:14:52Z

I reverted all changes made to any of the .mjs files. As I understand it, changes won't be necessary there.

howard-e

There's still some changes here that would require creating a separate PR on w3c/aria-at. That is everything that is any change inside client/resources/*

howard-e · 2024-03-18T21:04:45Z

client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx

@@ -17,7 +17,7 @@ import {
 } from '../../../resources/aria-at-test-io-format.mjs';
 import { TestWindow } from '../../../resources/aria-at-test-window.mjs';
 import { evaluateAtNameKey } from '../../../utils/aria.js';
-import commandsJson from '../../../resources/commands.json';
+import commandsJson from '../../../resources/commandsV1.json';


This is a v2 commands file. Files that look like the following is v2

howard-e · 2024-03-18T21:05:26Z

client/components/TestRenderer/index.jsx

@@ -25,7 +25,7 @@ import OutputTextArea from './OutputTextArea';
 import AssertionsFieldset from './AssertionsFieldset';
 import UnexpectedBehaviorsFieldset from './UnexpectedBehaviorsFieldset';
 import supportJson from '../../resources/support.json';
-import commandsJson from '../../resources/commands.json';
+import commandsJson from '../../resources/commandsV1.json';


This is a v2 commands file

howard-e · 2024-03-18T21:18:17Z

server/scripts/import-tests/index.js

    );

    try {
        // Commands path info for v2 format
        const commandsV2Path = pathToFileURL(
-            path.join(testsDirectory, 'commands.json')
+            path.join(testsDirectory, 'commandsV1.json')


This would be commandsV2.json. But for this change to be valid, a PR on aria-at must also move aria-at/tests/commands.json to aria-at/tests/commandsV2.json, otherwise this path is invalid and subsequent imports would be wrong.

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

server/scripts/import-tests/index.js

@@ -327,7 +327,7 @@ const updateJsons = async () => {
            JSON.stringify(flattenObject(commandsV2Parsed), null, 4)
        );
    } catch (error) {
-        console.error('commands.json for v2 test format may not exist');
+        console.error('commandsV1.json for v2 test format may not exist');


Right, it could (should?) say commandsV2.json does not exist instead or some variation of that.

howard-e · 2024-05-20T17:47:48Z

Overwritten by changes in #1064

Update commands.json file

996b3f0

Paul-Clue requested review from howard-e and alflennik February 26, 2024 19:42

alflennik requested changes Feb 28, 2024

View reviewed changes

Paul-Clue added 2 commits March 4, 2024 08:51

Revert some unnecessary changes

0ca7369

Change commands version in path

7e74af0

howard-e requested changes Mar 18, 2024

View reviewed changes

howard-e changed the base branch from main to development April 4, 2024 22:25

howard-e closed this May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update commands.json file to commandsV1.js #936

Update commands.json file to commandsV1.js #936

Paul-Clue commented Feb 26, 2024 •

edited by ccanash

Loading

alflennik left a comment

alflennik Feb 28, 2024

howard-e Mar 18, 2024

alflennik Feb 28, 2024

alflennik Feb 28, 2024

howard-e Mar 5, 2024

alflennik Feb 28, 2024

alflennik Feb 28, 2024

alflennik Feb 28, 2024

alflennik Feb 28, 2024

alflennik Feb 28, 2024

howard-e Mar 5, 2024

Paul-Clue commented Mar 14, 2024

howard-e left a comment

howard-e Mar 18, 2024

howard-e Mar 18, 2024

howard-e Mar 18, 2024

howard-e Mar 18, 2024

howard-e commented May 20, 2024

		@@ -1,4 +1,4 @@
		const commands = require('../../resources/commands.json');
		const commands = require('../../resources/commandsV1.json');

		@@ -126,8 +126,8 @@ export async function loadCollectedTestAsync(testRoot, testFileName) {
		const collectedTestResponse = await fetch(`${testRoot}/${testFileName}`);

Update commands.json file to commandsV1.js #936

Update commands.json file to commandsV1.js #936

Conversation

Paul-Clue commented Feb 26, 2024 • edited by ccanash 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Clue commented Mar 14, 2024

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

howard-e commented May 20, 2024

Paul-Clue commented Feb 26, 2024 •

edited by ccanash

Loading