Repository navigation
Consider setting up Gold for flutter/tests #71437
Description
I'm not sure if this is feasible. I'm hoping @Hixie and/or @Piinks will know.
When a 3p customer test (e.g. flutter_svg) fails on a golden file, perhaps because of minor and visually acceptable change in Skia, it requires updating the golden in the downstream repo and coordinating that change with an engine roll.
It would be far preferable if Skia gold, or something like it, could be used instead so that reviewers could triage these changes (with approval from the 3p owner of the package).
Activity
dnfield commentedon Nov 30, 2020
See #71436
dnfield commentedon Nov 30, 2020
/cc @zanderso as well - my thought is this would lead to a better workflow for engine sheriffs, but there may be some other way to handle this as well.
Piinks commentedon Nov 30, 2020
I think this would require our customers to create and manage their own Skia Gold instances (they are unique to a repository) and grant us permission to triage image changes.
dnfield commentedon Nov 30, 2020
If I wanted to do that for flutter_svg (or any 3p repo), is there documentation on how to do it?
Is it something we could conceivably do for the tests repo and tie the instance to the tests repository?
Piinks commentedon Nov 30, 2020
There is documentation at https://github.com/google/skia-buildbot/tree/master/golden in the read me
I don't think so, since changes to customer tests have to be associated with commits in the customer repo, not the flutter/tests repo. Otherwise customers would not be able to update their images while working on their own packages. Skia Gold associates images with commits in a given repository for that instance.
Right now, we clone a customer repo and run the tests. Customers could be managing their golden files any number of ways - we haven't specified rules around this type of test in flutter/tests.
dnfield commentedon Nov 30, 2020
That makes sense. I'm a little worried the burden of that is high for 3p customers who are not already using gold. In particular, every commit to flutter/flutter would end up using up GCP credits on a 3p customer's account. But maybe there's some other way to make this easier on rollers and customers...
Piinks commentedon Nov 30, 2020
I agree completely. I think it would be an undue burden.
Piinks commentedon Dec 1, 2020
Thinking about this a bit more, golden files are not considered breaking changes in flutter/flutter. I wonder if we should not allow them in flutter/tests since they are non-breaking. That would certainly free up the workflow.
dnfield commentedon Dec 1, 2020
That would make flutter_svg a little sad - the majority of tests there are golden tests.
Piinks commentedon Dec 2, 2020
But if goldens aren't considered breaking - why include them? 🤔
Especially with the challenges we've had today trying to update them due to an engine change. That change is not going through breaking change procedure, because it is not a breaking change. But it creates an excruciating break in the system that closes the tree.
I wonder, to what end do we include golden file tests in flutter/tests if not to just break the tree? What is the purpose of including golden files in flutter/tests?
Context:
flutter/tests#68
#71436
#71496
#71519
flutter/tests#70
#71498
dnfield commentedon Dec 2, 2020
I'm not sure I agree that a golden file changing isn't considered a breaking change. My understanding is that if one of our upstream dependencies (e.g. Skia) changes a golden in a way that isn't deemed to be a bug (e.g. minor pixel differences in anti-aliasing or gradient or font rendering that carries some other benefit), we don't consider it a breaking change for Flutter (because really it's a breaking change for that upstream dependency). So the change breaking flutter_svg's tests isn't a breaking change, and my job as the maintainer is to make sure it gets updated so that I can still pass on master.
That said, if Flutter broke rendering in a way that was only caught by a golden test, and we agreed it was not a bug but a feature, it would be a breaking change right?
All of that can be true and we could still decide to ban goldens from the 3p test repo, but we definitely deal with this elsewhere in the Google internal repo anyway.
I agree we need a better process for this. I opened this bug naively hoping that Skia Gold could help that process. Banning golden tests from the tests repo would also make this specific problem go away, but if we do that we should communicate it to customers and update the customer testing script(s) to enforce it.
4 remaining items
Hixie commentedon Dec 2, 2020
(And for my third comment of the day...)
I don't really understand the problem here though. What is different about updating golden tests vs any other test?
Piinks commentedon Dec 2, 2020
I think the issue here is that if a customer golden file test is broken by an engine roll, the roll gets stuck, and when the customer updates their golden file to unblock the engine roll, the tree breaks until the engine rolls in.
We were able to coordinate landing changes yesterday as it was just one customer package that was affected, but that's not going to scale very well.
Piinks commentedon Dec 2, 2020
We still support versioning golden files, like in the flutter/goldens days. Maybe we can encourage customers to version their golden files so the tree doesn't break.
Hixie commentedon Dec 2, 2020
The engine roll is not exempt from the breaking change policy. The correct behaviour is to revert the breaking change, make it opt-in, write a migration guide, opt people in, land the change, remove the opt-in from the customer tests, then remove the opt in, just like with any breaking change.
Unless we're talking specifically about upstream changes form Skia? (In which case maybe we should open a conversation with the Skia team about the impact of their breaking changes on our customers, and maybe they can included these tests in their CI.)
Piinks commentedon Dec 2, 2020
Yeah, it was traced to a Skia roll.
dnfield/flutter_svg#461
Hixie commentedon Dec 2, 2020
Maybe we can come up with some metric whereby each individual pixel is allowed to vary by some tiny amount? That would still allow my 5x5-box example to trigger a failure while allowing subtle shifts to be considered fine?
dnfield commentedon Dec 2, 2020
The reason I mentioned gold to begin with is that it could help us triage these things much more quickly and easily. Today the process ends up being:
And then reverts get painful.
Another option is to just temporarily disable the repo, which can lead to other problems (e.g. if some other breakage is introduced in the mean time).
If there was some way to set up gold so that it was registered to the tests repo, and had some "reasonable" amount of usage, that would help triaging this (the engine roll itself could choose to update the goldens).
Piinks commentedon Dec 2, 2020
I don't think that this is possible, or at least I haven't thought of a way yet.
If we set up a Gold instance for flutter/tests, it will relate specifically to commits in that repo. Not commits in customer/package. Not commits in flutter/flutter. It would not work like the engine rolling into flutter/flutter and triaging the new images for instance flutter/flutter.
Images are checked into Gold with commits, so without a commit in flutter/tests, new images could not be checked in.
Even if it could work, customers would have to integrate their tests with Gold and give us permission to approve changes - which doesn't feel right. I don't know that we can be the arbiter of someone else's golden files.
Furthermore, customers would not be able to work on their own package and their own golden files independently if we are managing them in a Gold instance based in a separate repo. It would effectively put the same burden on the customers to land multiple commits in order to land their own changes in their own product. And we would still have to step through the same process on the framework side. 😞
dnfield commentedon Dec 2, 2020
We could still allow customers to decide on approval, it would just be in gold (or something like gold anyway) and more manageable.
Wouldn't they still have their own golden files however they want to manage them, just distinct from the gold instance?
Piinks commentedon Dec 2, 2020
I feel I may not be conveying how Gold works very well. 😆
Gold cannot track images across multiple repositories.
dnfield commentedon Dec 3, 2020
I'm speaking of a hypothetical gold that can do this - whether by adding that feature to gold, or finding something else. I have no idea how feasible that is :)