-
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 16 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 |
---|---|---|
@@ -0,0 +1,47 @@ | ||
'use strict'; | ||
|
||
const { regenerateResultsAndRecalculateHashes } = require('./utils'); | ||
/** @type {import('sequelize-cli').Migration} */ | ||
module.exports = { | ||
async up(queryInterface, Sequelize) { | ||
const updateV2TestsToIncludeEmptyAssertionExceptions = | ||
async transaction => { | ||
const testPlanVersions = await queryInterface.sequelize.query( | ||
`SELECT id, tests FROM "TestPlanVersion" WHERE metadata->>'testFormatVersion' = '2'`, | ||
{ | ||
type: Sequelize.QueryTypes.SELECT, | ||
transaction | ||
} | ||
); | ||
await Promise.all( | ||
testPlanVersions.map(async ({ id, tests }) => { | ||
const updatedTests = JSON.stringify( | ||
tests.map(test => { | ||
test.assertions = test.assertions.map( | ||
assertion => ({ | ||
...assertion, | ||
assertionExceptions: [] | ||
}) | ||
); | ||
return test; | ||
}) | ||
); | ||
await queryInterface.sequelize.query( | ||
`UPDATE "TestPlanVersion" SET tests = ? WHERE id = ?`, | ||
{ replacements: [updatedTests, id], transaction } | ||
); | ||
}) | ||
); | ||
}; | ||
|
||
return queryInterface.sequelize.transaction(async transaction => { | ||
await updateV2TestsToIncludeEmptyAssertionExceptions(transaction); | ||
|
||
// Recalculate the hashes | ||
await regenerateResultsAndRecalculateHashes( | ||
queryInterface, | ||
transaction | ||
); | ||
}); | ||
} | ||
}; |
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