-
Notifications
You must be signed in to change notification settings - Fork 116
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
Chore: adding e2e tests for Thumbnails #915
Chore: adding e2e tests for Thumbnails #915
Conversation
Verified that @ConradJChan has signed the CLA. Thanks for the pull request! |
@@ -50,7 +50,7 @@ | |||
<button class="bp-btn-plain bp-btn-loading-download bp-is-invisible"></button> | |||
</div> | |||
</div> | |||
<div class="bp-content"> | |||
<div class="bp-content" data-testid="bp-content"> |
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.
Are we stripping out data-testid like in elements?
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.
Yeah we are, so this will get stripped out but html elements created via javascript won't. But when talking to @jstoffan this wasn't an issue because stripping these out is mainly to decrease bloat in the bundle.
|
||
/* eslint-disable */ | ||
const getThumbnail = (pageNum) => cy.get(`.bp-thumbnail[data-bp-page-num=${pageNum}]`); | ||
const showDocumentPreview = (enableThumbnailsSidebar) => { |
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.
Passing in an object with a named enableThumbnailsSidebar
property makes this a bit easier to read.
test/support/commands.js
Outdated
@@ -4,11 +4,17 @@ Cypress.Commands.add('getPreviewPage', (pageNum) => { | |||
cy | |||
.get(`.page[data-page-number=${pageNum}]`) | |||
.as('previewPage') | |||
.find('[data-testid="page-loading-indicator"]') | |||
// Adding 10s timeout here because there's no reliable way to wait for |
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.
Using the loading indicator seems reliable, but the document just might take longer to load/render than the default timeout allows.
test/support/commands.js
Outdated
@@ -19,6 +25,8 @@ Cypress.Commands.add('showPreview', (token, fileId, options) => { | |||
cy.window().then((win) => { | |||
win.loadPreview(options); | |||
}); | |||
} else { | |||
cy.getByTestId('load-preview').click(); |
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 needed or can we just remove the if (options)
check above and allow win.loadPreview(options)
to be used for all showPreview
invocations. It seems like it would be the same effect.
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.
Yeah that's cleaner let me change that
test/support/commands.js
Outdated
@@ -4,22 +4,26 @@ Cypress.Commands.add('getPreviewPage', (pageNum) => { | |||
cy | |||
.get(`.page[data-page-number=${pageNum}]`) | |||
.as('previewPage') | |||
.find('[data-testid="page-loading-indicator"]') | |||
// Adding 10s timeout here because sometimes it takes more than the Cypress |
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.
Looks like the duration in the comment is outdated. Probably easier to just remove it?
Modified the test fixture
index.html
to have an explicit load preview button instead of loading it when submitting the token or file id. This is to make it easier when using it during e2e Cypress testing to invoke it with preview options.