-
Notifications
You must be signed in to change notification settings - Fork 40
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
SDK-1884: Cypress SDK not wrapping A11Y commands appropriately #949
Conversation
osho-20
commented
Dec 3, 2024
•
edited
Loading
edited
- Fixed Cypress A11y Scan
- Added launch Options
@@ -6,6 +6,15 @@ const browserStackLog = (message) => { | |||
} | |||
|
|||
const commandsToWrap = ['visit', 'click', 'type', 'request', 'dblclick', 'rightclick', 'clear', 'check', 'uncheck', 'select', 'trigger', 'selectFile', 'scrollIntoView', 'scroll', 'scrollTo', 'blur', 'focus', 'go', 'reload', 'submit', 'viewport', 'origin']; | |||
const commandToOverwrite = ['visit', 'click', 'type', 'request', 'dblclick', 'rightclick', 'clear', 'check', 'uncheck', 'select', 'trigger', 'selectFile', 'scrollIntoView', 'scrollTo', 'blur', 'focus', 'go', 'reload', 'submit', 'viewport', 'origin']; |
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.
any reason for removing scroll
from the list ?
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.
scroll is not a default cypress function. This error was occuring.
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 verified across all supported cy versions ?
@@ -18,7 +27,7 @@ new Promise(async (resolve, reject) => { | |||
return win.document.querySelector("#accessibility-automation-element"); | |||
} | |||
|
|||
function waitForScannerReadiness(retryCount = 30, retryInterval = 100) { | |||
function waitForScannerReadiness(retryCount = 100, retryInterval = 100) { |
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 retryCount
number intentional ? Why 100 ?
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.
yes a11y team told to retry for 10 sec other framework have same count.
@@ -337,9 +329,11 @@ Cypress.Commands.add('performScan', () => { | |||
} | |||
cy.window().then(async (win) => { | |||
browserStackLog(`Performing accessibility scan`); | |||
await performScan(win); | |||
cy.wrap(performScan(win), {timeout:40000}); |
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.
This sets timeout, which can break the test if mochaTimeout is less than 40s. should we also handle mocha timeout ?
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.
if the response from performScan take less time no timeout will occur. Eventually if response does not come and mocha timeout occurs it should be considered as timeout.
81e2cc9
to
48c4784
Compare
bin/helpers/utils.js
Outdated
} | ||
const header = JSON.parse(base64UrlDecode(parts[0])); | ||
const payload = JSON.parse(base64UrlDecode(parts[1])); | ||
return { header, payload }; |
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.
why to parse header if we are not using
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.
also let's not return it if not used
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.
i made it a generic function to get used in future if need headers
RUN_SCA |
@@ -29,9 +69,11 @@ new Promise(async (resolve, reject) => { | |||
"Accessibility Automation Scanner is not ready on the page." | |||
) | |||
); | |||
return; |
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.
shouldn't promise be resolved?
@@ -43,6 +85,7 @@ new Promise(async (resolve, reject) => { | |||
function onScanComplete() { | |||
win.removeEventListener("A11Y_SCAN_FINISHED", onScanComplete); | |||
resolve(); | |||
return; |
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.
shouldn't promise be resolved?
@@ -111,7 +159,8 @@ new Promise((resolve) => { | |||
.then(getSummary) | |||
.catch((err) => { | |||
resolve(); | |||
}); | |||
return; |
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.
resolving do we need return?
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.
no
launchOptions.extensions.push(ally_path); | ||
if(!utils.isUndefined(payload) && !utils.isUndefined(payload.a11y_core_config) && payload.a11y_core_config.domForge === true) { |
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.
significance of these?
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.
other ticket SDK-1907
@@ -6,19 +6,59 @@ const browserStackLog = (message) => { | |||
} | |||
|
|||
const commandsToWrap = ['visit', 'click', 'type', 'request', 'dblclick', 'rightclick', 'clear', 'check', 'uncheck', 'select', 'trigger', 'selectFile', 'scrollIntoView', 'scroll', 'scrollTo', 'blur', 'focus', 'go', 'reload', 'submit', 'viewport', 'origin']; | |||
const commandToOverwrite = ['visit', 'click', 'type', 'request', 'dblclick', 'rightclick', 'clear', 'check', 'uncheck', 'select', 'trigger', 'selectFile', 'scrollIntoView', 'scrollTo', 'blur', 'focus', 'go', 'reload', 'submit', 'viewport', 'origin']; | |||
const performModifiedScan = (originalFn, Subject, stateType, ...args) => { |
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.
please add some comments explaining the below function for future ref
function runCutomizedCommand() { | ||
if (!Subject) { | ||
let orgS1, orgS2, cypressCommandSubject; | ||
if((orgS2 = (orgS1 = cy).subject) !==null && orgS2 !== void 0){ | ||
cypressCommandSubject = orgS2.call(orgS1); | ||
} | ||
else{ | ||
cypressCommandSubject = null; | ||
} | ||
customChaining.then(()=> cypressCommandSubject).then(() => {originalFn(...args)}); | ||
} | ||
else { | ||
let orgSC1, orgSC2, timeO1, cypressCommandChain, setTimeout; | ||
if((timeO1 = args.find(arg => arg !== null && arg !== void 0 ? arg.timeout : null)) !== null && timeO1 !== void 0) { | ||
setTimeout = timeO1.timeout; | ||
} | ||
else { | ||
setTimeout = null; | ||
} | ||
if((orgSC1 = (orgSC2 = cy).subjectChain) !== null && orgSC1 !== void 0){ | ||
cypressCommandChain = orgSC1.call(orgSC2); | ||
} | ||
else { | ||
cypressCommandChain = null; | ||
} | ||
customChaining.performScanSubjectQuery(cypressCommandChain, setTimeout).then({timeout: 30000}, (newSubject) => originalFn(...changeSub(args, stateType, newSubject))); |
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.
function runCutomizedCommand() { | |
if (!Subject) { | |
let orgS1, orgS2, cypressCommandSubject; | |
if((orgS2 = (orgS1 = cy).subject) !==null && orgS2 !== void 0){ | |
cypressCommandSubject = orgS2.call(orgS1); | |
} | |
else{ | |
cypressCommandSubject = null; | |
} | |
customChaining.then(()=> cypressCommandSubject).then(() => {originalFn(...args)}); | |
} | |
else { | |
let orgSC1, orgSC2, timeO1, cypressCommandChain, setTimeout; | |
if((timeO1 = args.find(arg => arg !== null && arg !== void 0 ? arg.timeout : null)) !== null && timeO1 !== void 0) { | |
setTimeout = timeO1.timeout; | |
} | |
else { | |
setTimeout = null; | |
} | |
if((orgSC1 = (orgSC2 = cy).subjectChain) !== null && orgSC1 !== void 0){ | |
cypressCommandChain = orgSC1.call(orgSC2); | |
} | |
else { | |
cypressCommandChain = null; | |
} | |
customChaining.performScanSubjectQuery(cypressCommandChain, setTimeout).then({timeout: 30000}, (newSubject) => originalFn(...changeSub(args, stateType, newSubject))); | |
function runCutomizedCommand() { | |
if (!Subject) { | |
let cypressCommandSubject = (cy.subject?.call(cy)) ?? null; | |
customChaining.then(() => cypressCommandSubject).then(() => { originalFn(...args); }); | |
} else { | |
let setTimeout = args.find(arg => arg?.timeout)?.timeout ?? null; | |
let cypressCommandChain = (cy.subjectChain?.call(cy)) ?? null; | |
customChaining.performScanSubjectQuery(cypressCommandChain, setTimeout).then({ timeout: 30000 }, newSubject => originalFn(...changeSub(args, stateType, newSubject))); | |
} | |
} |
The base branch was changed.
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.
Approved for base branch change