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

Added tool to easily check golden diffs locally. #50654

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

gaaclarke
Copy link
Member

This addresses the problem where people feel like they have to upload a PR to get diffs from Skia Gold. This should work for most workflows.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@gaaclarke
Copy link
Member Author

This could have been an external tool, it's so fundamental and tiny though I wanted to integrate it with the engine repo.

@matanlurey
Copy link
Contributor

I'd like to see this in tools/*, and follow the same structure as tools in that directory (including tests).

That being said, I also need that tool to exist (flutter/flutter#143459) - so I'm happy to have you merge this and then I can take that part over, up to you.

@gaaclarke
Copy link
Member Author

gaaclarke commented Feb 14, 2024

Yea, this happened because I started writing it in bash, then realized writing it in Dart would be better.

That being said, I also need that tool to exist (flutter/flutter#143459) - so I'm happy to have you merge this and then I can take that part over, up to you.

What did you mean by this? What's this have to do with that other proposed tool? Just that you'd be in there anyways? I'm happy to make it a more fleshed out tool. This was so tiny I was hoping this would be all that's required.

@matanlurey
Copy link
Contributor

Oh sorry I misunderstood what this is/isn't. Sgtm, I think having this is strictly better than not.

I do think we should put it in tools/ just to be consistent and not start accumulating Dart scripts all over.

@gaaclarke
Copy link
Member Author

I do think we should put it in tools/ just to be consistent and not start accumulating Dart scripts all over.

Done. I added all the stuff to write tests. I didn't add one, I can if we want. It requires some noodling with directories and mocking which adds extra complexity to the script.

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 15, 2024
@auto-submit auto-submit bot merged commit b7103bc into flutter:main Feb 15, 2024
28 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2024
jason-simmons added a commit to jason-simmons/flutter that referenced this pull request Feb 16, 2024
2024-02-15 30870216+gaaclarke@users.noreply.github.com Added tool to easily check golden diffs locally. (flutter/engine#50654)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 4bbf2060b008 to 12d0b7fac4c3 (2 revisions) (flutter/engine#50689)
2024-02-15 johnoneil@users.noreply.github.com Provide a matrix inverse shim for GLES 2.0. (flutter/engine#50545)
2024-02-15 jonahwilliams@google.com [iOS] Ensure FlutterMetalLayer has correct backpressure. (flutter/engine#50486)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 682f0e1e7e77 to 4bbf2060b008 (3 revisions) (flutter/engine#50686)
2024-02-15 103135467+sealesj@users.noreply.github.com Pin OSV-Scanner reusable workflow (flutter/engine#50649)
2024-02-15 whesse@google.com Add support for dart_src GN variable to flutter_frontend_server build (flutter/engine#50685)
2024-02-15 6718144+renancaraujo@users.noreply.github.com fix: consider array size on canvaskit shader data (flutter/engine#49754)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 1277910beec9 to 682f0e1e7e77 (1 revision) (flutter/engine#50683)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 85ab600a9519 to 1277910beec9 (2 revisions) (flutter/engine#50682)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 16, 2024
2024-02-15 30870216+gaaclarke@users.noreply.github.com Added tool to easily check golden diffs locally. (flutter/engine#50654)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 4bbf2060b008 to 12d0b7fac4c3 (2 revisions) (flutter/engine#50689)
2024-02-15 johnoneil@users.noreply.github.com Provide a matrix inverse shim for GLES 2.0. (flutter/engine#50545)
2024-02-15 jonahwilliams@google.com [iOS] Ensure FlutterMetalLayer has correct backpressure. (flutter/engine#50486)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 682f0e1e7e77 to 4bbf2060b008 (3 revisions) (flutter/engine#50686)
2024-02-15 103135467+sealesj@users.noreply.github.com Pin OSV-Scanner reusable workflow (flutter/engine#50649)
2024-02-15 whesse@google.com Add support for dart_src GN variable to flutter_frontend_server build (flutter/engine#50685)
2024-02-15 6718144+renancaraujo@users.noreply.github.com fix: consider array size on canvaskit shader data (flutter/engine#49754)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 1277910beec9 to 682f0e1e7e77 (1 revision) (flutter/engine#50683)
2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 85ab600a9519 to 1277910beec9 (2 revisions) (flutter/engine#50682)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants