Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Pulled out dir contents golden tool #50703

Merged
merged 10 commits into from
Feb 16, 2024
Merged

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Feb 15, 2024

fixes flutter/flutter#143459

This also starts using it as part of scenario_app.

Testing: Is part of the testing infrastructure.

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.

@gaaclarke gaaclarke changed the title Dir diff tool Pulled out dir contents golden tool Feb 16, 2024
@gaaclarke gaaclarke marked this pull request as ready for review February 16, 2024 19:03
@gaaclarke gaaclarke requested a review from matanlurey February 16, 2024 19:03
await step('Check output files...', () async {
// TODO(gaaclarke): We should move this into dir_contents_diff.
_withTemporaryCwd(contentsGolden, () {
final int exitCode = dirContentsDiff(basename(contentsGolden), screenshotPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

This prints out the patch relative to //testing/ which is annoying but not the end of the world. We should clean it up at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, in that it uses print (or similar) on a failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that it is executing git diff -p as a subprocess and printing out the output from that. When you use git diff -p the patch that is generated is relative to the CWD. We don't have the path to the repository at this point. We just have to path to the golden file. Another gotcha is that git diff will generate absolute paths if you run it from a CWD outside of a repo, which is no good. I looked for a while to find a flag to git diff that would fix it but gave up. If you are looking for some light reading, don't check out git help diff haha

Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

LGTM, could you just edit the README for scenario_app/bin and scenario_app/android to mention this new capability and the golden file/how to update it?

await step('Check output files...', () async {
// TODO(gaaclarke): We should move this into dir_contents_diff.
_withTemporaryCwd(contentsGolden, () {
final int exitCode = dirContentsDiff(basename(contentsGolden), screenshotPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, in that it uses print (or similar) on a failure?

@gaaclarke
Copy link
Member Author

LGTM, could you just edit the README for scenario_app/bin and scenario_app/android to mention this new capability and the golden file/how to update it?

Done. Updated //testing/scenario_app/README.md since the information is apropos //testing/scenario_app/run_android_tests.sh. The tool itself should be printing out information how to apply the patch. For the dart tool, there is help from the arg parser.

@matanlurey
Copy link
Contributor

re-LGTM.

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 16, 2024
@auto-submit auto-submit bot merged commit c4fe6f0 into flutter:main Feb 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 16, 2024
…143615)

flutter/engine@2eed3fb...c4fe6f0

2024-02-16 [email protected] Pulled out dir contents golden tool (flutter/engine#50703)
2024-02-16 [email protected] Roll Dart SDK from 21b9ee6f0a52 to fa66195a3814 (3 revisions) (flutter/engine#50732)
2024-02-16 [email protected] Roll Skia from 2919d86cad12 to 6ae5032133d0 (8 revisions) (flutter/engine#50729)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

Ensure screenshots are taken and uploaded to Skia gold, or fail the test
2 participants