Handle FormatException from SkiaGoldClient#143755
Conversation
github-actions
bot
added
a: tests
|
Hmm but I thought I turned off all gold checks? Has this happened recently? |
|
The backtrace has this line, which indicates that all the environment checks above have failed: https://github.com/flutter/flutter/blob/master/packages/flutter_goldens/lib/flutter_goldens.dart#L46 Is the sense of this check backwards? https://github.com/flutter/flutter/blob/master/packages/flutter_goldens/lib/flutter_goldens.dart#L428. That is, should it be checking that we are on the main or master branch? |
|
ahh interesting. I wouldn't think the default comparator would try to talk to skia gold? see also: https://flutter-review.googlesource.com/c/recipes/+/54930 |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
I think... IIRC, the framework smoke tests in flutter/engine intentionally use FlutterLocalFileComparator, which first makes a blank (not referencing a real test) request to Gold using https://flutter-gold.skia.org/json/v2/latestpositivedigest/$traceID to see if we can actually access it, so then golden images can be retrieved for testing. So I am wondering if something changed on Gold's side that made this request not work anymore.
It looks like the engine tree is green currently, so I'd like to find out from the Gold team if they made a change before we just on a format error, it could be all local testing does not work.
Hmm but I thought I turned off all gold checks? Has this happened recently?
@jonahwilliams golden tests for flutter/engine should not be running based on https://flutter-review.googlesource.com/c/recipes/+/54930, but the comparator is still initialized for testing.
| 'SocketException occurred, could not reach Gold. ' | ||
| 'Switching to FlutterSkippingGoldenFileComparator.', | ||
| ); | ||
| } on FormatException catch (_) { |
There was a problem hiding this comment.
I've not seen this error before from Gold in this context.
@kjlubick have there been any changes to https://flutter-gold.skia.org/json/v2/latestpositivedigest/$traceID?
There was a problem hiding this comment.
No changes. A socket exception seems like something in the network stack/path. 🤷
There was a problem hiding this comment.
Ok thanks. I was referring to the FormatException, but it sounds like everything is stable! :)
There was a problem hiding this comment.
Maybe a reply got truncated? Thus JSON parsing failed?
|
Also FYI @Hixie who has been rewriting this logic in another PR |
zanderso
dismissed
jonahwilliams’s stale review
Dismissing review to avoid this landing with open questions.
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
With flutter/engine being green still, it seems like this was maybe a flake? I think it is ok to land. There are no changes on Gold's end, and if local testing does breakdown due to a format exception, there will be plenty output when folks run tests locally, so I doubt we will miss it. :)
LGTM. Thanks @zanderso.
4 participants
Seen in https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20Framework%20Smoke%20Tests/17183/overview closing the engine tree.