Skip to content

Minimal screenshot tool. #4074

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Minimal screenshot tool. #4074

wants to merge 1 commit into from

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Jul 4, 2025

  • The test_browser.dart and the screenshot_util.dart is a slightly adjusted copy from dart-lang/pub-dev. I think in the long term we may want to extract it into a utility package that could be imported by both projects.
  • Uses an end2end test and observes its output with a local http server (thus the index.json fetching also works).
  • Right now only a few random pages are selected, we should probably handpick a few more, or maybe even make it crawl the entire output, not sure about that part.
  • Screenshotting the search dropdown is TBD.
  • Comparison tool is TBD, I think we could just start with the tool from pub-dev (or again, extract it into a shared package).

@isoos isoos requested a review from sigurdm July 4, 2025 17:21
@@ -51,6 +57,8 @@ void main() {
// Set up the pub metadata for our test packages.
runPubGet(testPackageToolError.path);
runPubGet(_testSkyEnginePackage.path);

if (isScreenshotDirSet) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

intentional?

Copy link
Collaborator Author

@isoos isoos Jul 7, 2025

Choose a reason for hiding this comment

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

Yes. We want to run screenshots only when the output directory is set, regular tests should run without if - at least for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the "then" branch is empty...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, my bad, mobile view of the code needs more attention.

No, this is not intentional, a leftover, will fix it once I'm with a computer.

@@ -27,7 +27,10 @@ dev_dependencies:
dart_style: ^3.0.0
lints: ">=5.0.0 <7.0.0"
matcher: ^0.12.15
puppeteer: ^3.18.0
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to discuss if we want a non-dart-team dependency here (even if it is a dev-dependency)
cc @szakarias wdyt?

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