Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
running tests by target name, selecting web rendering backend
  • Loading branch information
nturgut committed Nov 10, 2020
commit 8117c928b77ce23fcfa239933d00c28e09bde533
157 changes: 111 additions & 46 deletions lib/web_ui/dev/integration_tests_manager.dart
Original file line number Diff line number Diff line change
Expand Up @@ -122,27 +122,41 @@ class IntegrationTestsManager {
final List<String> blockedTests =
blockedTestsListsMap[getBlockedTestsListMapKey(_browser)] ?? <String>[];

// The following loops over the contents of the directory and saves an
// expected driver file name for each e2e test assuming any dart file
// not ending with `_test.dart` is an e2e test.
// Other files are not considered since developers can add files such as
// README.
for (io.File f in entities) {
final String basename = pathlib.basename(f.path);
if (!basename.contains('_test.dart') && basename.endsWith('.dart')) {
// Do not add the basename if it is in the `blockedTests`.
if (!blockedTests.contains(basename)) {
e2eTestsToRun.add(basename);
} else {
print('INFO: Test $basename is skipped since it is blocked for '
'${getBlockedTestsListMapKey(_browser)}');
// If no target is specified run all the tests.
if (IntegrationTestsArgumentParser.instance.testTarget.isEmpty) {
// The following loops over the contents of the directory and saves an
// expected driver file name for each e2e test assuming any dart file
// not ending with `_test.dart` is an e2e test.
// Other files are not considered since developers can add files such as
// README.
for (io.File f in entities) {
final String basename = pathlib.basename(f.path);
if (!basename.contains('_test.dart') && basename.endsWith('.dart')) {
// Do not add the basename if it is in the `blockedTests`.
if (!blockedTests.contains(basename)) {
e2eTestsToRun.add(basename);
} else {
print('INFO: Test $basename is skipped since it is blocked for '
'${getBlockedTestsListMapKey(_browser)}');
}
}
}
print(
'INFO: In project ${directory} ${e2eTestsToRun.length} tests to run.');
} else {
// If a target is specified it will run regardless of if it's blocked or
// not. There will be an info note to warn the developer.
final String targetTest =
IntegrationTestsArgumentParser.instance.testTarget;
final io.File file =
entities.singleWhere((f) => pathlib.basename(f.path) == targetTest);
final String basename = pathlib.basename(file.path);
if (blockedTests.contains(basename)) {
print('INFO: Test $basename do not run on CI environments.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say "Skipping test $basename" (for consistency with flutter test and pub run test), unless I'm misunderstanding what blocked test means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Felt will run the test.

The warning is for letting developer know that CI environment do not run the test. I added more information to the warning.

}
e2eTestsToRun.add(basename);
}

print(
'INFO: In project ${directory} ${e2eTestsToRun.length} tests to run.');

int numberOfPassedTests = 0;
int numberOfFailedTests = 0;

Expand All @@ -160,27 +174,52 @@ class IntegrationTestsManager {
: {'profile', 'release'};
}
for (String fileName in e2eTestsToRun) {
for (String mode in buildModes) {
if (!blockedTestsListsMapForModes[mode].contains(fileName)) {
final bool testResults =
await _runTestsInMode(directory, fileName, mode: mode);
if (testResults) {
numberOfPassedTests++;
} else {
numberOfFailedTests++;
}
}
}
await _runTestsTarget(directory, fileName, buildModes);
}
final int numberOfTestsRun = numberOfPassedTests + numberOfFailedTests;

final int numberOfTestsRun = _numberOfPassedTests + _numberOfFailedTests;

print('INFO: ${numberOfTestsRun} tests run. ${numberOfPassedTests} passed '
'and ${numberOfFailedTests} failed.');
return numberOfFailedTests == 0;
}

int _numberOfPassedTests = 0;
int _numberOfFailedTests = 0;

Future<void> _runTestsTarget(
io.Directory directory, String target, Set<String> buildModes) async {
for (String mode in buildModes) {
if (!blockedTestsListsMapForModes[mode].contains(target)) {
// Run tests on html backend if no backend is selected or only
// `html` backend is selected.
if (!_renderingBackendSelected || _renderingBackendHtml) {
final bool htmlTestResults =
await _runTestsInMode(directory, target, mode: mode);
if (htmlTestResults) {
_numberOfPassedTests++;
} else {
_numberOfFailedTests++;
}
}
// Run the same test with canvaskit rendering backend if `html`
// backend is not specifically selected.
if (!_renderingBackendSelected || !_renderingBackendHtml) {
final bool canvaskitTestResults = await _runTestsInMode(
directory, target,
mode: mode, canvaskitBackend: true);
if (canvaskitTestResults) {
_numberOfPassedTests++;
} else {
_numberOfFailedTests++;
}
}
}
}
}

Future<bool> _runTestsInMode(io.Directory directory, String testName,
{String mode = 'profile'}) async {
{String mode = 'profile', bool canvaskitBackend = false}) async {
String executable =
_useSystemFlutter ? 'flutter' : environment.flutterCommand.path;
Map<String, String> enviroment = Map<String, String>();
Expand All @@ -189,6 +228,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),
Expand Down Expand Up @@ -313,6 +356,12 @@ class IntegrationTestsManager {
bool get _buildModeSelected =>
!IntegrationTestsArgumentParser.instance.buildMode.isEmpty;

bool get _renderingBackendSelected =>
!IntegrationTestsArgumentParser.instance.renderingBackend.isEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the rendering backend. felt should run in one mode (it could provide --web-renderer, just like flutter run and flutter drive do. This would simplify the felt code and user-facing feature set, and make it consistent with the Flutter CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I stated above, I'll leave the code as it is.

As opposed to Flutter CLI this code is mainly used by a few developers frequently changing the web engine code. The following scenario is too common and adding a stress to LUCI resources:

  • Developer A completes the design and run the tests with only: felt test
  • Tests are passing, developer assumes all is fine and push the change
  • PR is created, developer will have to wait for a while for PR to get assigned LUCI resources.
  • After waiting 20+ minutes for the result, on LUCI, firefox and ios-safari browsers are failing since the developer didn't run felt test --browser=firefox and felt test --browser=ios-safari.
    Negatives:
  • LUCI resources are consumed for a test case which could have been seen on easily on local.
  • Developer lost 30 minutes to see the PR is not complete, probably already switched context to a new task.

As principle, it's better to design a tool to show the error as early as possible in the development cycle. Therefore, I'm writing code to cover all 3 debug modes and 2 rendering backends. If a developer want to test their initial idea in one mode they can still pass arguments to test in a specific mode (html+debug for example).

Copy link
Contributor

Choose a reason for hiding this comment

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

I have bad news 😅 I already don't run felt test, and mostly rely on unit-tests locally, unless I suspect an integration test may be affected.

Realistically, we will reach a point when it's no longer practical to run all tests locally before sending a PR (just like in flutter/flutter it's impractical to run all shards in test.dart). In fact, even today if one is on Linux there's no way to run Safari tests locally. So I think standalone felt test is not likely to scale as the project matures.


bool get _renderingBackendHtml =>
IntegrationTestsArgumentParser.instance.renderingBackend == 'html';

/// Validate the given `browser`, `platform` combination is suitable for
/// integration tests to run.
bool validateIfTestsShouldRun() {
Expand Down Expand Up @@ -447,13 +496,14 @@ class IntegrationTestsArgumentParser {
/// If not specified, these tests will run using 'debug, profile, release'
/// modes on Chrome and will run using 'profile, release' on other browsers.
///
///
/// In order to skip a test for one of the modes, add the test to the
/// `blockedTestsListsMapForModes` list for the relevant compile mode.
String buildMode;

/// Whether the rendering backend to be used is html.
/// Whether to use html or canvaskit backend.
///
/// If set to false canvaskit rendering backend will be used.
bool useHtmlBackend;
/// If not set html rendering backend will be used.
String renderingBackend;

void populateOptions(ArgParser argParser) {
argParser
Expand All @@ -468,28 +518,39 @@ class IntegrationTestsArgumentParser {
)
..addOption('build-mode',
defaultsTo: '',
help: 'Flutter supports three modes when building your app. The By '
'default debug, profile, release modes will be used one by one '
'on Chrome and profile, release modes will be used for other '
'browsers. If a build mode is selected tests will only be run '
'using that mode. ')
..addFlag('html-backend',
defaultsTo: true,
help: 'Default rendering backend is HTML. If this flag is set to '
'false canvaskit backend will be used for rendering when running '
'integration tests.');
help: 'Flutter supports three modes when building your app. This '
'option sets the build mode for the integration tests. '
'By default an integration test will sequentially run on '
'multiple modes. All three modes (debug, release, profile) are '
'used for Chrome. Only profile, release modes will be used for '
'other browsers. In other words, if a build mode is selected '
'tests will only be run using that mode. '
'See https://flutter.dev/docs/testing/build-modes for more '
'details on the build modes.')
..addOption('rendering-backend',
defaultsTo: 'html',
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`'
' are the only available options. ');
}

/// Populate browser with results of the arguments passed.
void parseOptions(ArgResults argResults) {
testTarget = argResults['target'] as String;
buildMode = argResults['build-mode'] as String;
if (buildMode.toLowerCase() != 'debug' &&
if (!buildMode.isEmpty &&
buildMode.toLowerCase() != 'debug' &&
buildMode.toLowerCase() != 'profile' &&
buildMode.toLowerCase() != 'release') {
throw ArgumentError('Unexpected build mode: $buildMode');
}
useHtmlBackend = argResults['html-backend'] as bool;
renderingBackend = argResults['rendering-backend'] as String;
if (!renderingBackend.isEmpty &&
renderingBackend.toLowerCase() != 'html' &&
renderingBackend.toLowerCase() != 'canvaskit') {
throw ArgumentError('Unexpected rendering backend: $renderingBackend');
}
}
}

Expand Down Expand Up @@ -537,6 +598,10 @@ const Map<String, List<String>> blockedTestsListsMap = <String, List<String>>{
};

/// Tests blocked for one of the build modes.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is more common to call such tests "skipped" rather than "blocked".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is outside of the scope of this work. It has been a while since we added 'blocked' test concept. Blocked word is introduced as a replacement to 'blacklist' a while ago.

///
/// If a test is not suppose to run for one of the modes also add that test
/// to the corresponding list.
// TODO(nurhan): Remove the failing test after fixing.
const Map<String, List<String>> blockedTestsListsMapForModes =
Copy link
Contributor

Choose a reason for hiding this comment

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

It's still weird that one needs to recompile felt in order to change the list of tests they want to run. I'd prefer a YAML file that lives closer to the test code itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's been a while we added blocked tests logic, it seems like the request is outside of this PR. I create another issue and address that change there.

<String, List<String>>{
'debug': [
Expand Down
6 changes: 3 additions & 3 deletions lib/web_ui/dev/test_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,9 @@ class TestCommand extends Command<bool> with ArgUtils {
}

Future<bool> runIntegrationTests() async {
// if(!_testPreparationReady) {
// await _prepare();
// }
if(!_testPreparationReady) {
await _prepare();
}
return IntegrationTestsManager(
browser, useSystemFlutter, doUpdateScreenshotGoldens)
.runTests();
Expand Down