Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple settings being defined in *-commands.csv #1125

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support multiple settings being defined in *-commands.csv #1125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
29e33f8
Support space separated settings and render result for createAllTests
howard-e Aug 29, 2024
49b0924
Fix for collected test not properly handling presentationNumber when …
howard-e Sep 3, 2024
af94498
Support additional settings on review page
howard-e Sep 3, 2024
57f2343
Update snapshot
howard-e Sep 3, 2024
542256a
Remove unnecessary attributes
howard-e Sep 4, 2024
51cd2bc
'additional' as a better descriptor
howard-e Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,526 changes: 1,090 additions & 436 deletions __test__/__snapshots__/createAllTests.test.js.snap
View file
Open in desktop

Large diffs are not rendered by default.

325 changes: 65 additions & 260 deletions __test__/__snapshots__/createReviewPages.test.js.snap
View file
Open in desktop

Large diffs are not rendered by default.

37 changes: 20 additions & 17 deletions lib/data/parse-command-csv-row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,24 @@ function parseCommandCSVRowV2({ commands }, commandsJson) {
const commandIds = command.command.split(' ');
const commandKVs = findValuesByKeys(flattenedCommandsJson, [command.command]);

// Also account for 'modeless' AT configurations with 'defaultMode'
const [primarySetting, ...additionalSettings] = command.settings
? command.settings.split(' ')
: ['defaultMode'];

const foundCommandIndex = commandsParsed.findIndex(
e =>
e.testId === command.testId &&
e.target.at.key === atTargetInfo.key &&
e.target.at.settings === primarySetting &&
(e.commands.length
? e.commands.every(
({ presentationNumber }) =>
parseInt(presentationNumber) === parseInt(command.presentationNumber)
)
: true)
);

const assertionExceptions = command.assertionExceptions.length
? sanitizeWhitespace(command.assertionExceptions).split(' ')
: [];
Expand All @@ -207,32 +225,17 @@ function parseCommandCSVRowV2({ commands }, commandsJson) {
})),
presentationNumber: Number(command.presentationNumber),
assertionExceptions,
additionalSettings,
}));

// Also account for 'modeless' AT configurations with 'defaultMode'
const settingsValue = command.settings || 'defaultMode';

const foundCommandIndex = commandsParsed.findIndex(
e =>
e.testId === command.testId &&
e.target.at.key === atTargetInfo.key &&
e.target.at.settings === settingsValue &&
(e.commands.length
? e.commands.every(
({ presentationNumber }) =>
parseInt(presentationNumber) === parseInt(command.presentationNumber)
)
: true)
);

// Add new parsed command since none exist
if (foundCommandIndex < 0) {
commandsParsed.push({
testId: command.testId,
target: {
at: {
...atTargetInfo,
settings: settingsValue,
settings: primarySetting,
},
},
commands,
Expand Down
24 changes: 14 additions & 10 deletions lib/data/parse-test-csv-row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,9 @@ function parseTestCSVRowV2({ tests, assertions, scripts, commands }) {
}

// Also account for 'modeless' AT configurations with 'defaultMode'
// TODO: Account for a list of settings as described by https://github.com/w3c/aria-at/wiki/Test-Format-Definition-V2#settings
const settingsValue = command.settings || 'defaultMode';
const [primarySetting, ...additionalSettings] = command.settings
? command.settings.split(' ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This structure with the primary setting as the first item in a space-separated collection feels a bit fragile. I'd rather have this as a distinct column. It feels like there is significance not surfaced by a quick read of the input. I assume that this is a design decision towards a better authoring experience so I support leaving as-is for now

: ['defaultMode'];

if (isEmptyObject(testsParsed[arrPosition])) {
testsParsed[arrPosition] = {
Expand All @@ -162,7 +163,7 @@ function parseTestCSVRowV2({ tests, assertions, scripts, commands }) {
ats: [
{
...atTargetInfo,
settings: settingsValue,
settings: primarySetting,
},
],
},
Expand All @@ -178,12 +179,12 @@ function parseTestCSVRowV2({ tests, assertions, scripts, commands }) {
} else {
if (
!testsParsed[arrPosition].target.ats.find(
e => e.key === atTargetInfo.key && e.settings === settingsValue
e => e.key === atTargetInfo.key && e.settings === primarySetting
)
)
testsParsed[arrPosition].target.ats.push({
...atTargetInfo,
settings: settingsValue,
settings: primarySetting,
});
}
}
Expand All @@ -203,25 +204,28 @@ function parseTestCSVRowV2({ tests, assertions, scripts, commands }) {
return (
testId === commandInfo.testId &&
command === commandInfo.command &&
settings === commandInfo.settings &&
settings === commandInfo.settings && // the primary settings value
presentationNumber === commandInfo.presentationNumber
);
}

for (const command of commands) {
testsParsed.forEach(test => {
const [primarySetting, ...additionalSettings] = command.settings
? command.settings.split(' ')
: ['defaultMode'];

if (
command.testId === test.testId &&
test.target.ats.some(
at => at.key === key && at.settings === (command.settings || 'defaultMode')
)
test.target.ats.some(at => at.key === key && at.settings === primarySetting)
) {
const commandInfo = {
testId: command.testId,
command: command.command,
settings: command.settings || 'defaultMode',
settings: primarySetting,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition of settings isn't really appropriate in the presence of additionalSettings. Will this change also be reflected in the test format definition? What would be the ramifications of renaming this property to primarySetting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition of settings isn't really appropriate in the presence of additionalSettings ... renaming this property to primarySetting?

I agree! I figured as much when sharing the initial gist on what's different here. I proposed mainSetting then and have since used primarySetting here as well which feels more appropriate. Otherwise, it certainly presents a bit of confusion without this context.

Will this change also be reflected in the test format definition?

I don't think it necessary. Seems to be something to consider as an implementation detail.

What would be the ramifications ...

A widespread refactor across the references in this repository, aria-at-app (as well as some migration or backward compatibility work) and also in aria-at-automation-harness and related resources I assume. That will certainly need a bit of coordination that I'm uneasy trying to get that done now. Do you feel strongly otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's an implementation detail for the Community Group at large. That said, the amount of inter-project coordination needed to support change makes it feel like a public-facing design.

This change creates a gap between what the CG calls "settings" and what code maintainers call "settings", and to me, that's a problem which is distinct from the internally inconsistent names.

I dropped in from w3c/aria-at-automation-harness#73 to verify that my understanding of the situation was accurate. I trust your judgement when it comes to coordinating changes; I'd only advocate for prioritizing that refactoring sooner rather than later!

presentationNumber: Number(command.presentationNumber),
assertionExceptions: command.assertionExceptions,
additionalSettings,
};

// Test level commandInfo, useful for getting assertionExceptions and
Expand Down
51 changes: 48 additions & 3 deletions scripts/test-reviewer/processCollectedTests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const processCollectedTests = ({ collectedTest, commandsAPI, atKey, mode, task,
commandsValuesForInstructions,
modeInstructions = undefined;

// TODO: Standardize naming on settings instead of outdated 'mode'
let additionalSettings = [];

/** @type {CollectedTestAT} */
const at = commandsAPI.isKnownAT(atKey);
const defaultConfigurationInstructions = unescapeHTML(
Expand Down Expand Up @@ -50,6 +53,15 @@ const processCollectedTests = ({ collectedTest, commandsAPI, atKey, mode, task,
})
: undefined;

function findCommandInfo(assertionForCommand) {
return collectedTest.commandsInfo?.[at.key]?.find(
c =>
c.command === assertionForCommand.key &&
c.settings === assertionForCommand.settings &&
c.presentationNumber === assertionForCommand.presentationNumber
);
}

/**
* Check default assertion priorities to verify if there is a priority exception and return the
* updated list of assertions
Expand All @@ -64,9 +76,10 @@ const processCollectedTests = ({ collectedTest, commandsAPI, atKey, mode, task,
// Check to see if there is any command info exceptions for current at key
const foundCommandInfo = collectedTest.commandsInfo?.[at.key]?.find(
c =>
c.assertionExceptions.includes(assertion.assertionId) &&
c.command === assertionForCommand.key &&
c.settings === assertionForCommand.settings
c.settings === assertionForCommand.settings &&
c.presentationNumber === assertionForCommand.presentationNumber &&
c.assertionExceptions.includes(assertion.assertionId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

);

if (foundCommandInfo) {
Expand Down Expand Up @@ -105,6 +118,36 @@ const processCollectedTests = ({ collectedTest, commandsAPI, atKey, mode, task,
assertionsForCommandsInstructions.length &&
typeof assertionsForCommandsInstructions[0] === 'object'
) {
// Check for additional settings
const foundCommandInfos = assertionsForCommandsInstructions.map(findCommandInfo);
assertionsForCommandsInstructions = assertionsForCommandsInstructions.map(
(assertionForCommand, index) => {
const foundCommandInfo = foundCommandInfos[index];

const additionalSettingsExpanded = [];
for (const additionalSetting of foundCommandInfo.additionalSettings) {
if (!additionalSettings.includes(additionalSetting))
additionalSettings.push(additionalSetting);

const expandedSettings = {
settings: additionalSetting,
settingsText: at.settings[additionalSetting].screenText,
};

// Update value text if additional settings exist
assertionForCommand.value =
assertionForCommand.value.slice(0, -1) + ` and ${expandedSettings.settingsText})`;

additionalSettingsExpanded.push(expandedSettings);
}

return {
...assertionForCommand,
additionalSettingsExpanded,
additionalSettings: foundCommandInfo.additionalSettings,
};
}
);
commandsValuesForInstructions = assertionsForCommandsInstructions.map(each => each.value);
} else {
// V1 came in as array of strings
Expand Down Expand Up @@ -149,7 +192,9 @@ const processCollectedTests = ({ collectedTest, commandsAPI, atKey, mode, task,
// An error will occur if there is no data for an AT, ignore it
}

for (const atMode of mode.split('_')) {
// Create unique set
const foundAtModes = [...new Set([...mode.split('_'), ...additionalSettings])];
for (const atMode of foundAtModes) {
// TODO: If there is ever need to explicitly show the instructions
// for an AT with the default mode active
// const atSettingsWithDefault = {
Expand Down
Loading
Loading