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

Upgrades to felt (running on multiple modes, multiple backends, single test target option) #22260

Merged
nturgut merged 16 commits into from
Nov 12, 2020
Merged

Upgrades to felt (running on multiple modes, multiple backends, single test target option) #22260

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
40848e0
testing running the tests on all build modes
Nov 3, 2020
f2213f3
don't run debug mode on other browsers
Nov 3, 2020
75465a3
fix platform message test failures
Nov 3, 2020
eae835f
some cleanup. change dispose platform channel message
Nov 3, 2020
5889d0c
adding flags to control the integration tests better with felt
Nov 3, 2020
8117c92
running tests by target name, selecting web rendering backend
Nov 4, 2020
3d7e145
fix conditions
Nov 4, 2020
0f13714
carrying some conditions to helper methods. Adding comments
Nov 4, 2020
dfb5353
create a blocked list for failing canvaskit test
Nov 4, 2020
78fad8e
parse parameters before all integration tests
Nov 4, 2020
9b5f830
Give better warning to developers for tests that are blocked for CI
Nov 9, 2020
2598808
address some reviwer comments (more remains)
Nov 9, 2020
1812790
remove named parameters
Nov 9, 2020
73fab26
also run with auto mode
Nov 10, 2020
c997661
add verbose option
Nov 11, 2020
f373ba6
reduce the number of tests running. skip url_test for now
Nov 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix conditions
  • Loading branch information
nturgut committed Nov 10, 2020
commit 3d7e14589b8966221649277827409f28126c634c
65 changes: 39 additions & 26 deletions lib/web_ui/dev/integration_tests_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,19 +160,8 @@ class IntegrationTestsManager {
int numberOfPassedTests = 0;
int numberOfFailedTests = 0;

Set<String> buildModes;
if (_buildModeSelected) {
final String mode = IntegrationTestsArgumentParser.instance.buildMode;
if (mode == 'debug' && _browser != 'chrome') {
throw ToolException('Debug mode is only supported for Chrome.');
} else {
buildModes = <String>{mode};
}
} else {
buildModes = _browser == 'chrome'
? {'debug', 'profile', 'release'}
: {'profile', 'release'};
}
final Set<String> buildModes = _getBuildModes();

for (String fileName in e2eTestsToRun) {
await _runTestsTarget(directory, fileName, buildModes);
}
Expand Down Expand Up @@ -228,13 +217,10 @@ class IntegrationTestsManager {
}
final IntegrationArguments arguments =
IntegrationArguments.fromBrowser(_browser);
final List<String> testArgs = arguments.getTestArguments(testName, mode);
if (canvaskitBackend) {
testArgs.add('--web-renderer=canvaskit');
}
final int exitCode = await runProcess(
executable,
arguments.getTestArguments(testName, mode),
arguments.getTestArguments(testName, mode,
isCanvaskitBackend: canvaskitBackend),
workingDirectory: directory.path,
environment: enviroment,
);
Expand All @@ -251,6 +237,23 @@ class IntegrationTestsManager {
}
}

Set<String> _getBuildModes() {
Set<String> buildModes;
if (_buildModeSelected) {
final String mode = IntegrationTestsArgumentParser.instance.buildMode;
if (mode == 'debug' && _browser != 'chrome') {
throw ToolException('Debug mode is only supported for Chrome.');
} else {
buildModes = <String>{mode};
}
} else {
buildModes = _browser == 'chrome'
? {'debug', 'profile', 'release'}
: {'profile', 'release'};
Copy link
Contributor

@yjbanov yjbanov Nov 5, 2020

Choose a reason for hiding this comment

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

I think the built-in knowledge of which browsers should use which modes is adding complexity without much benefit. For example, one might attempt to run a test in Edge, but will need to change felt code in order to do it.

I would instead run in one mode only and in the CI script invoke felt multiple time with different modes, perhaps even in different subshards.

Copy link
Contributor Author

@nturgut nturgut Nov 9, 2020

Choose a reason for hiding this comment

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

  1. I already wrote a design with running multiple shards. Felt can be invoked with multiple modes. Yet the default will be all modes.
  2. I think it only requires knowledge of whether that build mode is supported in a browser. Felt still only tests for legacy Edge, therefore it only testing for profile and release mode will be ok. I will leave the tests like this since if a developer runs: felt test I want them to see all that is failing and passing instead of running the multiple times with different build modes.

}
return buildModes;
}

/// Validate the directory has a `pubspec.yaml` file and a `test_driver`
/// directory.
///
Expand Down Expand Up @@ -405,14 +408,16 @@ abstract class IntegrationArguments {
}
}

List<String> getTestArguments(String testName, String mode);
List<String> getTestArguments(String testName, String mode,
{bool isCanvaskitBackend = false});
Copy link
Contributor

@yjbanov yjbanov Nov 5, 2020

Choose a reason for hiding this comment

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

Renderer can now take three values: html, canvaskit, and auto.

(this comment applies throughout the PR)

Copy link
Contributor Author

@nturgut nturgut Nov 9, 2020

Choose a reason for hiding this comment

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

Thanks for letting me know :) I'll also add that!


String getCommandToRun(String testName, String mode);
}

/// Arguments to give `flutter drive` to run the integration tests on Chrome.
class ChromeIntegrationArguments extends IntegrationArguments {
List<String> getTestArguments(String testName, String mode) {
List<String> getTestArguments(String testName, String mode,
{bool isCanvaskitBackend = false}) {
return <String>[
'drive',
'--target=test_driver/${testName}',
Expand All @@ -423,10 +428,12 @@ class ChromeIntegrationArguments extends IntegrationArguments {
if (isLuci) '--chrome-binary=${preinstalledChromeExecutable()}',
'--headless',
'--local-engine=host_debug_unopt',
if (isCanvaskitBackend) '--web-renderer=canvaskit',
];
}

String getCommandToRun(String testName, String mode) {
String getCommandToRun(String testName, String mode,
{bool isCanvaskitBackend = false}) {
String statementToRun = 'flutter drive '
'--target=test_driver/${testName} -d web-server --$mode '
'--browser-name=chrome --local-engine=host_debug_unopt';
Expand All @@ -440,7 +447,8 @@ class ChromeIntegrationArguments extends IntegrationArguments {

/// Arguments to give `flutter drive` to run the integration tests on Firefox.
class FirefoxIntegrationArguments extends IntegrationArguments {
List<String> getTestArguments(String testName, String mode) {
List<String> getTestArguments(String testName, String mode,
{bool isCanvaskitBackend = false}) {
return <String>[
'drive',
'--target=test_driver/${testName}',
Expand All @@ -450,18 +458,21 @@ class FirefoxIntegrationArguments extends IntegrationArguments {
'--browser-name=firefox',
'--headless',
'--local-engine=host_debug_unopt',
if (isCanvaskitBackend) '--web-renderer=canvaskit',
];
}

String getCommandToRun(String testName, String mode) =>
String getCommandToRun(String testName, String mode,
{bool isCanvaskitBackend = false}) =>
'flutter ${getTestArguments(testName, mode).join(' ')}';
}

/// Arguments to give `flutter drive` to run the integration tests on Safari.
class SafariIntegrationArguments extends IntegrationArguments {
SafariIntegrationArguments();

List<String> getTestArguments(String testName, String mode) {
List<String> getTestArguments(String testName, String mode,
{bool isCanvaskitBackend = false}) {
return <String>[
'drive',
'--target=test_driver/${testName}',
Expand All @@ -470,10 +481,12 @@ class SafariIntegrationArguments extends IntegrationArguments {
'--$mode',
'--browser-name=safari',
'--local-engine=host_debug_unopt',
if (isCanvaskitBackend) '--web-renderer=canvaskit',
];
}

String getCommandToRun(String testName, String mode) =>
String getCommandToRun(String testName, String mode,
{bool isCanvaskitBackend = false}) =>
'flutter ${getTestArguments(testName, mode).join(' ')}';
}

Expand Down Expand Up @@ -528,7 +541,7 @@ class IntegrationTestsArgumentParser {
'See https://flutter.dev/docs/testing/build-modes for more '
'details on the build modes.')
..addOption('rendering-backend',
defaultsTo: 'html',
defaultsTo: '',
help: 'By default both `html` and `canvaskit` rendering backends are '
' tested when running integration tests. If this option is set '
' only one of these backends will be used. `canvaskit` and `html`'
Expand Down