-
Notifications
You must be signed in to change notification settings - Fork 21
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
[UIE-205] Puppeteer upgrade (version 22.7.1) #5117
Conversation
@@ -2,7 +2,7 @@ const PuppeteerEnvironment = require('jest-environment-puppeteer'); | |||
const { parse } = require('path'); | |||
const { maybeSaveScreenshot, savePageContent } = require('./utils/integration-utils'); | |||
|
|||
class JestCircusEnvironment extends PuppeteerEnvironment { | |||
class JestCircusEnvironment extends PuppeteerEnvironment.default { |
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.
Puppeteer changed how this is exported.
// Workaround for issue when accessing cross domain iframes in puppeteer while not headless: | ||
// https://github.com/GoogleChrome/puppeteer/issues/4960 | ||
args: (({ headfull, devtools }) => { | ||
const flag = '--disable-features=site-per-process'; |
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.
None of this is needed anymore-- I ran the IA analysis test locally (which interacts with an iframe) with both HEADFUL and DEVTOOLS, and it passed.
} | ||
return []; | ||
})({ headfull: process.env.HEADLESS === 'false', devtools: process.env.DEVTOOLS === 'true' }), | ||
protocolTimeout: 720_000, // 12 minutes |
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.
At some point, Puppeteer decreased their default protocolTimeout. I needed to increase this for the IA analysis tests, which wait for a very long time for Jupyter or RStudio to launch and be ready to interact with.
@@ -8,21 +8,21 @@ | |||
"test-flakes": "FLAKES=true yarn test-local --runInBand" | |||
}, | |||
"devDependencies": { | |||
"jest": "^27.4.3", | |||
"jest-environment-puppeteer": "^6.0.3", | |||
"jest": "^29.7.0", |
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 increased these all to their most recent version.
}, | ||
"dependencies": { | ||
"@axe-core/puppeteer": "^4.6.0", | ||
"date-fns": "^2.24.0", | ||
"google-auth-library": "^7.9.2", | ||
"lodash": "^4.17.21", | ||
"p-retry": "^4.6.1", | ||
"puppeteer": "^13.1.3", | ||
"puppeteer": "22.7.1", |
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 is the last Puppeteer version that works with the IA iframes (using Chrome version 124.0.6367.78). All later versions I tried failed in the same way: broken computer icon in the iframe and a message about being unable to access dev leonardo.
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.
@jdcanas just pinging you in case you might know, but it sounds like we eventually will need to rewrite our IA UI Integration tests to be better compatible with pupeteer?
@@ -40,7 +40,7 @@ const billingProjectsPage = (testPage, testUrl) => { | |||
|
|||
assertChartValue: async (number, workspaceName, category, cost) => { | |||
// This checks the accessible text for chart values. | |||
await testPage.waitForXPath(`(//*[@role="img"])[contains(@aria-label,"${number}. Workspace ${workspaceName}, ${category}: ${cost}.")]`); | |||
await testPage.waitForSelector(`[role="img"][aria-label*="${number}. Workspace ${workspaceName}, ${category}: ${cost}."]`); |
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.
waitForXPath
has been removed.
@@ -30,14 +30,14 @@ const testFindWorkflowFn = _.flow( | |||
without these lines it will redirect to dev-Terra even if started out at localhost:3000-Terra */ | |||
if (testUrl === 'http://localhost:3000') { | |||
await findElement(page, clickable({ textContains: 'Yes' })); | |||
const yesButtonHrefDetails = (await page.$x('//a[contains(text(), "Yes")]/@href'))[0]; | |||
const yesButtonHrefDetails = (await page.$$('xpath///a[contains(text(), "Yes")]/@href'))[0]; |
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.
Rename of this method, and you have to specify 'xpath/' at the beginning if using an xpath.
}; | ||
|
||
const waitForNoModal = (page, { timeout = 30000 } = {}) => { | ||
return page.waitForXPath('//*[contains(@class, "ReactModal__Overlay")]', { hidden: true, timeout }); | ||
return page.waitForSelector('.ReactModal__Overlay', { hidden: true, 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.
Simplification
return page.waitForXPath(xpath, defaultToVisibleTrue(options)); | ||
}; | ||
|
||
const findErrorPopup = (page, options) => { |
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.
findErrorPopup
was unused.
@@ -364,25 +360,19 @@ const findInDataTableRow = (page, entityName, text) => { | |||
return findElement(page, elementInDataTableRow(entityName, text)); | |||
}; | |||
|
|||
const findButtonInDialogByAriaLabel = (page, ariaLabelText) => { |
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.
Unused.
}, | ||
"dependencies": { | ||
"@axe-core/puppeteer": "^4.6.0", | ||
"date-fns": "^2.24.0", | ||
"google-auth-library": "^7.9.2", | ||
"lodash": "^4.17.21", | ||
"p-retry": "^4.6.1", | ||
"puppeteer": "^13.1.3", | ||
"puppeteer": "22.7.1", |
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.
@jdcanas just pinging you in case you might know, but it sounds like we eventually will need to rewrite our IA UI Integration tests to be better compatible with pupeteer?
@LizBaldo @jdcanas Regarding Liz's comment, no, there isn't a way to rewrite the test. The issue is that the iframe itself is showing no content (just an unhappy computer icon) because the content is getting blocked. I believe it is due to specific newer versions of Chrome, and hopefully there is some flag we can pass during test startup that will prevent the issue. I just haven't figured out yet what it is… all the things I saw suggested on Stack Overflow didn't work. chromewebdata/:1 Refused to display 'https://leonardo.dsde-dev.broadinstitute.org/' in a frame because it set 'X-Frame-Options' to 'sameorigin'. UPDATE: this @mlilynolting @jdcanas @LizBaldo this is caused by the third-party cookie issue. Apparently newer versions of Chrome for Testing are enforcing that third-party cookies are not enabled. Is there a plan yet for how to handle this problem? I feel like there should be an option that I can pass to Chrome to disable the enforcement, but I have yet to figure it out. |
Jira Ticket: https://broadworkbench.atlassian.net/browse/UIE-205
This upgrades Puppeteer to the most recent version that I can where our tests that interact with iframes still work. Unfortunately, this doesn't address the
ws
security issue, but it gets us very close to a version that does (22.11.2 or possible 22.9.0). I will continue to see if I can find a workaround that allows us to bring in a newer version of Puppeteer, but we might as well upgrade to this version in the meantime.Note that starting with version 20, Chrome for Testing is used instead of Chromium.