Skip to content
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

Add some UI tests #19

Merged
merged 16 commits into from
Dec 5, 2024
Merged

Add some UI tests #19

merged 16 commits into from
Dec 5, 2024

Conversation

gjmooney
Copy link
Collaborator

@gjmooney gjmooney commented Nov 29, 2024

#7 Add some UI tests.

@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 2, 2024

please update snapshots

@gjmooney gjmooney marked this pull request as ready for review December 2, 2024 15:13
@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 2, 2024

please update snapshots

1 similar comment
@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 2, 2024

please update snapshots

@jtpio
Copy link
Collaborator

jtpio commented Dec 2, 2024

Looks like the snapshot update workflows is failing: https://github.com/TileDB-Inc/jupyter-iframe-commands/actions/runs/12122527111/job/33795905340

We will probably need to first fix the workflow and have the changes available on the main since that workflow runs from main.

Otherwise in the meantime we could grab the snapshots from the playwright report for this PR, and commit them directly.

@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 2, 2024

Looks like the snapshot update workflows is failing: https://github.com/TileDB-Inc/jupyter-iframe-commands/actions/runs/12122527111/job/33795905340

We will probably need to first fix the workflow and have the changes available on the main since that workflow runs from main.

Otherwise in the meantime we could grab the snapshots from the playwright report for this PR, and commit them directly.

Yea I'm fighting with this on my fork right now, I didn't want to keep spamming this repo 🫠

@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 3, 2024

please update snapshots

@jtpio
Copy link
Collaborator

jtpio commented Dec 3, 2024

Kicking CI after the snapshot update.

@jtpio jtpio closed this Dec 3, 2024
@jtpio jtpio reopened this Dec 3, 2024
@jtpio
Copy link
Collaborator

jtpio commented Dec 3, 2024

Looking at the UI tests report, it seems the snapshot is taken a bit too soon before JupyterLab has time to load properly:

jupyter-iframe-commands-ui-tests.webm

Maybe we can introduce an artificial delay, or look for a specific element in the JupyterLab Iframe before taking the snapshot (which would be more robust than relying on an arbitrary timeout).

@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 3, 2024

Maybe we can introduce an artificial delay, or look for a specific element in the JupyterLab Iframe before taking the snapshot (which would be more robust than relying on an arbitrary timeout).

I'm attempting to wait for the splash screen to be gone before taking the snapshot

const waitForApp = async (page: Page) => {
await page
.locator('#jupyterlab')
.contentFrame()
.locator('#jupyterlab-splash')
.waitFor({ state: 'detached' });
await page
.locator('#jupyterlab')
.contentFrame()
.locator('#galaxy')
.waitFor({ state: 'detached' });
await page
.locator('#jupyterlab')
.contentFrame()
.locator('#main-logo')
.waitFor({ state: 'detached' });
await page
.locator('#jupyterlab')
.contentFrame()
.locator('.jp-LauncherCard-icon')
.first()
.waitFor();
};
but that's not doing it apparently.

@jtpio
Copy link
Collaborator

jtpio commented Dec 3, 2024

Indeed, and this also looks similar to what Galata does by default: https://github.com/jupyterlab/jupyterlab/blob/a0733bf0ccc29fe8830b89cf557edfa14ffc9fd8/galata/src/fixtures.ts#L352-L355

@gjmooney gjmooney marked this pull request as draft December 4, 2024 14:37
@gjmooney
Copy link
Collaborator Author

gjmooney commented Dec 5, 2024

please update snapshots

@gjmooney gjmooney closed this Dec 5, 2024
@gjmooney gjmooney reopened this Dec 5, 2024
@gjmooney gjmooney marked this pull request as ready for review December 5, 2024 11:53
@gjmooney gjmooney requested a review from jtpio December 5, 2024 11:53
Copy link
Collaborator

@jtpio jtpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jtpio jtpio merged commit 1cb74a6 into main Dec 5, 2024
7 checks passed
@jtpio jtpio deleted the tests branch December 5, 2024 13:59
@jtpio jtpio mentioned this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants