- Notifications
You must be signed in to change notification settings - Fork133
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
test/end2end/dartdoc_test.dart Outdated
| runPubGet(testPackageToolError.path); | ||
| runPubGet(_testSkyEnginePackage.path); | ||
| if (isScreenshotDirSet) {} |
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.
intentional?
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.
Yes. We want to run screenshots only when the output directory is set, regular tests should run without if - at least for now.
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.
But the "then" branch is empty...
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.
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.
| dart_style:^3.0.0 | ||
| lints:">=5.0.0 <7.0.0" | ||
| matcher:^0.12.15 | ||
| puppeteer:^3.18.0 |
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.
We might need to discuss if we want a non-dart-team dependency here (even if it is a dev-dependency)
cc@szakarias wdyt?
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.
I was thinking to implement this as an external screenshotting tool, mostly because there is no natural integration point with browser-based tests like inpub-dev's integration tests, and maybepub-dev could reuse the tool as a library. wdyt?
test_browser.dartand thescreenshot_util.dartis a slightly adjusted copy fromdart-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.pub-dev(or again, extract it into a shared package).