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

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented May 19, 2022

@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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

@CaseyHillers
Copy link
Contributor

Can we use Google Testing to verify this change?

@zanderso
Copy link
Member

@CaseyHillers can you point to instructions on how to do that?

@dnfield
Copy link
Contributor Author

dnfield commented May 19, 2022

I started a TGP on cl/449791593, which contains this change.

@dnfield
Copy link
Contributor Author

dnfield commented May 19, 2022

Arrrgh. It wasn't synced properly and now I'm trying to fix that.

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 19, 2022
@dnfield
Copy link
Contributor Author

dnfield commented May 19, 2022

@CaseyHillers FWIW, engine contributors are not likely to go through the exercise of testing this. I don't know what we can do about avoiding this short of:

  • Building impeller internally
  • Adding FRoB support to the engine repo

Running a test on this was pretty trivial since I had a bad CL I could quickly clone and patch with this change. If I was working on some random impeller or Android change I would not think to do that and it may be much more difficult.

@Hixie
Copy link
Contributor

Hixie commented May 19, 2022

test-exempt: build configuration will be tested by target build environment

@dnfield dnfield merged commit 62d2ce3 into flutter:main May 19, 2022
@dnfield
Copy link
Contributor Author

dnfield commented May 19, 2022

@CaseyHillers there are a bunch of failures on my TGP but they all seem to be related to other problems with web or a different engine patch than the impeller one.

@CaseyHillers
Copy link
Contributor

By Google Testing, I meant the presubmit frob status. It looks like that doesn't run on the engine.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs tests platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants