-
Notifications
You must be signed in to change notification settings - Fork 48
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
Adding waitForTimeout/waitForSelector for assets discovery browser when JS is enabled #1715
Conversation
6a71cc4
to
9dd0779
Compare
9dd0779
to
24b65c8
Compare
|
d21f946
to
7c530ec
Compare
7c530ec
to
a710f44
Compare
|
a710f44
to
3f4b118
Compare
aca9bea
to
a2c68fc
Compare
packages/core/test/discovery.test.js
Outdated
it('waitForSelectorInsideBrowser should work', async () => { | ||
const page = await percy.browser.page(); | ||
spyOn(page, 'eval').and.callThrough(); | ||
waitForSelectorInsideBrowser(page, 'body', 30000); | ||
|
||
await percy.idle(); | ||
|
||
expect(page.eval).toHaveBeenCalledWith('await waitForSelector("body", 30000)'); | ||
}); |
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.
Move this test inside utils.test.js, as you are directly testing a function of utils.js
packages/core/test/discovery.test.js
Outdated
'[percy:core:discovery] Wait for 100ms timeout' | ||
])); | ||
}); | ||
it('waitForTimeout and waitForSelector are not called Js is disabled and domSnapshot is not present', async () => { |
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.
describe('when JS is enabled')
it('calls page.eval when selector is given')
- Check if page.eval is called for selector
describe('when JS is disabled')
it('not calls page.eval')
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.
Rest everything looks fine to me
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.
Lgtm 🌮 , Just update the tests please.
a2c68fc
to
e728df3
Compare
e728df3
to
51ecf8c
Compare
so now waitForTimeout and waitForSelector are at 2 places:
usage of both:
when it is passed at snapshot level =>
these options are used before a DOM snapshot is taken. The primary goal is to ensure that the page has fully loaded and any necessary DOM elements are present before capturing the state of the page. This mode is currently only supported when used with percy snapshot cli command.
when it is passed at discovery level and JS is enabled => in this phase, these options are used to wait for specific assets or network requests to be discovered and captured. The asset-discovery process often occurs during or after initial page load, and some resources may not be immediately available.
how client should use:
snapshot level => You should use these options at the snapshot level when you want to ensure the readiness of a page before it is captured. This ensures that no partial or incomplete content is included in the snapshot.
discovery level => You should use these options in the discovery level when you are facing issues with missing assets or incomplete network requests. By waiting for the selector or a set timeout, you give enough time for assets to load