-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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 0-level assertion priorities on TestRun page #863
Changes from 11 commits
69245f8
619a5fe
0c099f2
51ae9a5
7c464db
bde9219
c7d57fa
5c4ec6a
7de158b
3cbe58a
635cf65
318b7f4
0d5d165
a26a572
62ecce2
45d3ed6
af9a5b0
3d1fbcb
942223d
bba8ad4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -405,6 +405,8 @@ class CommandsInput { | |
commands.push(command); | ||
commandsAndSettings.push({ | ||
command, | ||
commandId, | ||
presentationNumber: Number(presentationNumber), | ||
settings: _atMode, | ||
settingsText: assistiveTech.settings?.[_atMode]?.screenText || 'default mode active', | ||
settingsInstructions: assistiveTech.settings?.[_atMode]?.instructions || [ | ||
|
@@ -767,6 +769,9 @@ class BehaviorInput { | |
|
||
const { commandsAndSettings } = commandsInput.getCommands({ task: json.task }, mode); | ||
|
||
// Use to determine assertionExceptions | ||
const commandsInfo = json.commandsInfo?.[at.key]; | ||
|
||
return new BehaviorInput({ | ||
behavior: { | ||
description: titleInput.title(), | ||
|
@@ -778,7 +783,31 @@ class BehaviorInput { | |
setupScriptDescription: json.setup_script_description, | ||
setupTestPage: json.setupTestPage, | ||
assertionResponseQuestion: json.assertionResponseQuestion, | ||
commands: commandsAndSettings, | ||
commands: commandsAndSettings.map(cs => { | ||
const foundCommandInfo = commandsInfo?.find( | ||
c => | ||
cs.commandId === c.command && | ||
cs.presentationNumber === c.presentationNumber && | ||
cs.settings === c.settings | ||
); | ||
if (!foundCommandInfo || !foundCommandInfo.assertionExceptions) return cs; | ||
|
||
// Only works for v2 | ||
let assertionExceptions = json.output_assertions.map(each => each.assertionId); | ||
foundCommandInfo.assertionExceptions.split(' ').forEach(each => { | ||
let [priority, assertionId] = each.split(':'); | ||
const index = assertionExceptions.findIndex(each => each === assertionId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||
|
||
priority = Number(priority); | ||
assertionExceptions[index] = priority; | ||
Comment on lines
+801
to
+802
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Under the assumption that "fewer mutable bindings is better", these statements could be combined to allow swapping the earlier There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good |
||
}); | ||
// Preserve default priority or update with exception | ||
assertionExceptions = assertionExceptions.map((each, index) => | ||
isNaN(each) ? json.output_assertions[index].priority : each | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't the members of |
||
); | ||
|
||
return { ...cs, assertionExceptions }; | ||
}), | ||
assertions: (json.output_assertions ? json.output_assertions : []).map(assertion => { | ||
// Tuple array [ priorityNumber, assertionText ] | ||
if (Array.isArray(assertion)) { | ||
|
@@ -788,7 +817,7 @@ class BehaviorInput { | |
}; | ||
} | ||
|
||
// { assertionId, priority, assertionStatement, assertionPhrase, refIds, commandInfo, tokenizedAssertionStatements } | ||
// { assertionId, priority, assertionStatement, assertionPhrase, refIds, tokenizedAssertionStatements } | ||
return { | ||
priority: assertion.priority, | ||
assertion: | ||
|
@@ -815,7 +844,7 @@ class BehaviorInput { | |
* @param {UnexpectedInput} data.unexpectedInput | ||
*/ | ||
static fromCollectedTestCommandsKeysUnexpected( | ||
{ info, target, instructions, assertions }, | ||
{ info, target, instructions, assertions, commands }, | ||
{ commandsInput, keysInput, unexpectedInput } | ||
) { | ||
// v1:info.task, v2: info.testId | v1:target.mode, v2:target.at.settings | ||
|
@@ -834,7 +863,28 @@ class BehaviorInput { | |
specificUserInstruction: instructions.raw || instructions.instructions, | ||
setupScriptDescription: target.setupScript ? target.setupScript.description : '', | ||
setupTestPage: target.setupScript ? target.setupScript.name : undefined, | ||
commands: commandsAndSettings, | ||
commands: commandsAndSettings.map(cs => { | ||
const foundCommandInfo = commands.find( | ||
c => cs.commandId === c.id && cs.settings === c.settings | ||
); | ||
if (!foundCommandInfo || !foundCommandInfo.assertionExceptions) return cs; | ||
|
||
// Only works for v2 | ||
let assertionExceptions = assertions.map(each => each.assertionId); | ||
foundCommandInfo.assertionExceptions.forEach(each => { | ||
let { priority, assertionId } = each; | ||
const index = assertionExceptions.findIndex(each => each === assertionId); | ||
|
||
priority = Number(priority); | ||
assertionExceptions[index] = priority; | ||
}); | ||
// Preserve default priority or update with exception | ||
assertionExceptions = assertionExceptions.map((each, index) => | ||
isNaN(each) ? assertions[index].priority : each | ||
); | ||
|
||
return { ...cs, assertionExceptions }; | ||
}), | ||
assertions: assertions.map( | ||
({ priority, expectation, assertionStatement, tokenizedAssertionStatements }) => { | ||
let assertion = tokenizedAssertionStatements | ||
|
@@ -1187,6 +1237,7 @@ export class TestRunInputOutput { | |
description: command.settings, | ||
text: command.settingsText, | ||
instructions: command.settingsInstructions, | ||
assertionExceptions: command.assertionExceptions, | ||
}, | ||
atOutput: { | ||
highlightRequired: false, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,7 @@ export class commandsAPI { | |
return { | ||
value, | ||
key, | ||
settings: mode, | ||
}; | ||
}) | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,16 +30,26 @@ const createTestResultSkeleton = ({ | |
id: scenarioResultId, | ||
scenarioId: scenario.id, | ||
output: null, | ||
assertionResults: test.assertions.map(assertion => { | ||
return { | ||
assertionResults: test.assertions | ||
// Filter out assertionResults which were marked with a 0-priority exception | ||
.filter(assertion => { | ||
return !assertion.assertionExceptions?.some( | ||
e => | ||
scenario.commands.find( | ||
c => | ||
c.id === e.commandId && | ||
c.atOperatingMode === e.settings | ||
) && e.priority === 'EXCLUDE' | ||
); | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment doesn't seem to acknowledge the first half of the filter expression, making me wonder whether I understand what's going on here. Would it be accurate to say, "Filter out assertionResults for the current scenario which were marked with a 0-priority exception"? Stylistically, I think that the closure nesting is reaching a point where it interferes with readability. While the in-line documentation you've suggested helps, breaking this logic out into a standalone function would afford us more room to describe what's going on, reduce the cognitive load (by formalizing the inputs), and limit distraction for folks seeking to understand /**
* Determine whether a given assertion belongs to a given scenario and includes
* at least one exception with a given priority.
*
* @param {string} priority
* @param {Scenario} scenario
* @param {Assertion} assertion
*/
const hasExceptionWithPriority = (priority, scenario, assertion) => {
return assertion.assertionExceptions?.some(
e =>
scenario.commands.find(
c =>
c.id === e.commandId &&
c.atOperatingMode === e.settings
) && e.priority === priority
);
}; Then invoking like this:
I'm only sharing something concrete as a means to demonstrate my point; I'm not too concerned with the specific factoring. For instance, a more purpose-built function would simplify the callsite even further (though it might be difficult to name it). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on this. I was able to take your suggestion and only apply minor changes, thanks! Done in bba8ad4 |
||
.map(assertion => ({ | ||
id: createAssertionResultId( | ||
scenarioResultId, | ||
assertion.id | ||
), | ||
assertionId: assertion.id, | ||
passed: null | ||
}; | ||
}), | ||
})), | ||
unexpectedBehaviors: null | ||
}; | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this taking advantage of a side effect or is this the strategy used for communicating that an assertion is excluded? It seems easy to misunderstand or forget. I would expect them to be passed prefiltered but I might be missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been some time now, so I'll have to look back into why exactly it was done that way. But from what I can remember, the
TestRenderer
still technically requires the index of that excluded assertion to properly determine which assertions are which, but at some point. It definitely had to be removed before saving and those excluded ones had no ids. That was a part of fulfilling this request with 5c4ec6a.So seems to be a bit of both.
This point in the code looks like the safest place to filter them out, as the single point before being passed to the server/db. The comment could be much clearer as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay. The background is helpful. I'll leave it to you as to whether you want to change the comment or address it any other way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in af9a5b0