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

October 1 package updates #12226

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

October 1 package updates #12226

wants to merge 2 commits into from

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Sep 30, 2024

Description

We can get rid of sinon entirely now, as Playwright finally supports manual system time control 🎉

Any existing sinon functionality has been ported to use Playwright's Clock API.

Otherwise, these are routine package updates.

Testing plan

  1. Ensure CI passes
  2. Run test-e2e-update locally and ensure appropriate screenshots are generated.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @ggetz!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor Author

ggetz commented Sep 30, 2024

@lukemckinstry Could you please do a review?

CI and running end-to-end tests locally should thoroughly cover these updates.

@lukemckinstry
Copy link
Contributor

the following snapshots appeared off (see filenames to tie back to test)

loads-animated-model-1-chromium-linux
loads-animated-model-1-chromium-linux

Made a separate issue for this one as discussed #12227
Apps-Sandcastle-gallery-AEC-Clipping-html-renders-1-chromium-linux
Apps-Sandcastle-gallery-AEC-Clipping-html-renders-1-chromium-linux

Apps-Sandcastle-gallery-ArcGIS-MapServer-html-renders-1-chromium-linux
Apps-Sandcastle-gallery-ArcGIS-MapServer-html-renders-1-chromium-linux

Apps-Sandcastle-gallery-ArcGIS-Tiled-Elevation-Terrain-html-renders-1-chromium-linux
Apps-Sandcastle-gallery-ArcGIS-Tiled-Elevation-Terrain-html-renders-1-chromium-linux

Apps-Sandcastle-gallery-Google-Earth-Enterprise-html-renders-1-chromium-linux
Apps-Sandcastle-gallery-Google-Earth-Enterprise-html-renders-1-chromium-linux

Apps-Sandcastle-gallery-I3S-3D-Object-Layer-html-renders-1-chromium-linux
Apps-Sandcastle-gallery-I3S-3D-Object-Layer-html-renders-1-chromium-linux

@lukemckinstry
Copy link
Contributor

one more to check

Apps-Sandcastle-gallery-I3S-Feature-Picking-html-renders-1-chromium-linux
Apps-Sandcastle-gallery-I3S-Feature-Picking-html-renders-1-chromium-linux

@ggetz
Copy link
Contributor Author

ggetz commented Sep 30, 2024

Thanks @lukemckinstry! For a bit of context, the *-Sandcastle-* tests simply load the initial frame of the corresponding Sandcastle.

loads-animated-model-1-chromium-linux

That should be correct, we hide the background in those model tests.

Apps-Sandcastle-gallery-AEC-Clipping-html-renders-1-chromium-linux

I'm not able to reproduce at all. I'm pretty sure this is unrelated to this PR, as per #12227.

Apps-Sandcastle-gallery-ArcGIS-MapServer-html-renders-1-chromium-linux

This is fine, see the Sandcastle example

Apps-Sandcastle-gallery-ArcGIS-Tiled-Elevation-Terrain-html-renders-1-chromium-linux

This is fine, see the Sandcastle example

Apps-Sandcastle-gallery-Google-Earth-Enterprise-html-renders-1-chromium-linux

Expected. This one doesn't do much without a local GEE server set up.

Apps-Sandcastle-gallery-I3S-3D-Object-Layer-html-renders-1-chromium-linux

This is fine, see the Sandcastle example. The top-down view doesn't look like much.

Apps-Sandcastle-gallery-I3S-Feature-Picking-html-renders-1-chromium-linux

This is fine, see the Sandcastle example. The top-down view doesn't look like much.

@lukemckinstry
Copy link
Contributor

thanks for the responses/explanations of the screenshots @ggetz
I think this is good to go once CI passes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants