Skip to content

Reland: Skia gold driver test#50160

Merged
blasten merged 2 commits intoflutter:masterfrom
blasten:reland-skia-gold
Feb 7, 2020
Merged

Reland: Skia gold driver test#50160
blasten merged 2 commits intoflutter:masterfrom
blasten:reland-skia-gold

Conversation

@blasten
Copy link
Copy Markdown

@blasten blasten commented Feb 5, 2020

Re-land for #49905.

The solution is to add import 'package:flutter_test/src/_goldens_web.dart'; to _scuba_test_config_web.dart.

@blasten blasten requested a review from chingjun February 5, 2020 02:15
@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 5, 2020
@goderbauer
Copy link
Copy Markdown
Member

/cc @Piinks

@Piinks
Copy link
Copy Markdown
Contributor

Piinks commented Feb 5, 2020

The solution is to add import 'package:flutter_test/src/_goldens_web.dart'; to _scuba_test_config_web.dart.

Do you mean for the roll?

@chingjun
Copy link
Copy Markdown
Contributor

chingjun commented Feb 6, 2020

I have three main concerns on this.

  1. Adding the requirement to a subset of package:flutter_test that part of it can't depend on dart:ui, in a library that has always been used in flutter environment, might add maintenance burden in the future.
  2. Conditionally imported files should expose the same interface, and the fact that _goldens_io.dart and _goldens_web.dart expose different interface is why we have the error internally. (Due to how dart analyzer works)
  3. Requiring that internal code to import a file from src/, especially one that is usually conditionally imported, also increase the likelihood that something to be broken in the future.

Discussed with @blasten just now. To resolve 2 and 3, we could split goldens.dart into two sets of files. goldens.dart, _golden_{io,web},dart, and web_comparator.dart, _web_comparator_{io,web}.dart. Both sets will export the same interface, and the class declaration of WebGoldenComparator will be moved to web_comparator.dart, so that we have the same interface on both web and VM.

I would also suggest that we think about issue 1 as well. We could, maybe add another entrypoint to make it clearer about the limitation, or move the requirement to somewhere more contained and less depended on, like package:flutter_golden.

@blasten
Copy link
Copy Markdown
Author

blasten commented Feb 6, 2020

@chingjun thanks for the suggestion. Thinking this through, it makes sense to just change the WebGoldenComparator interface, so the methods take dynamic types instead of types defined in dart:ui. I'm going to go with this approach since it's simpler. cc @jonahwilliams who had some concerns.

This reverts commit 9b3e163.
@blasten
Copy link
Copy Markdown
Author

blasten commented Feb 6, 2020

PTAL

Comment thread packages/flutter_test/lib/src/_goldens_web.dart Outdated
@blasten blasten requested a review from chingjun February 6, 2020 22:50
Copy link
Copy Markdown
Contributor

@chingjun chingjun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

// https://github.com/flutter/flutter/issues/49837
await expectLater(
driver.screenshot(),
bufferMatchesGoldenFile('red_square_driver_screenshot__$deviceModel.png'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering as I've been working on #49815, is there anything in the environment that indicates this is a driver test? Or what device it is running on?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not in the environment, unfortunately.

enableFlutterDriverExtension() has an assert WidgetsBinding.instance is _DriverBinding. Maybe we could make _DriverBinding public.

cc @jonahwilliams do you have any suggestions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can just provide a flag then, or include something in the file name. When we get down to the skia client I would like to specify in the parameters that this is a device test and/or the device it is running on, but it may not be necessary.

https://github.com/flutter/flutter/pull/49815/files#r376168906

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The test is always under a test_driver directory. Potentially that could be the heuristic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perfect. SGTM.

@blasten blasten merged commit 8c398a8 into flutter:master Feb 7, 2020
@blasten blasten deleted the reland-skia-gold branch February 7, 2020 01:14
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants