Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic reporting to the test runner #624

Merged
merged 8 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
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
38 changes: 29 additions & 9 deletions packages/patrol_cli/lib/src/features/drive/drive_command.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:ansi_styles/extension.dart';
import 'package:dispose_scope/dispose_scope.dart';
import 'package:freezed_annotation/freezed_annotation.dart';
import 'package:mason_logger/mason_logger.dart';
Expand Down Expand Up @@ -32,6 +33,8 @@ class DriveCommandConfig with _$DriveCommandConfig {
}) = _DriveCommandConfig;
}

const _defaultRepeats = 1;

class DriveCommand extends StagedCommand<DriveCommandConfig> {
DriveCommand({
required DeviceFinder deviceFinder,
Expand Down Expand Up @@ -114,7 +117,7 @@ class DriveCommand extends StagedCommand<DriveCommandConfig> {
'repeat',
abbr: 'n',
help: 'Repeat the test n times.',
defaultsTo: '1',
defaultsTo: '$_defaultRepeats',
);
}

Expand Down Expand Up @@ -175,9 +178,9 @@ class DriveCommand extends StagedCommand<DriveCommandConfig> {
throw const FormatException('`wait` argument is not an int');
}

var repeat = 1;
final int repeat;
try {
final repeatStr = argResults?['repeat'] as String? ?? '1';
final repeatStr = argResults?['repeat'] as String? ?? '$_defaultRepeats';
repeat = int.parse(repeatStr);
} on FormatException {
throw const FormatException('`repeat` argument is not an int');
Expand Down Expand Up @@ -222,15 +225,12 @@ class DriveCommand extends StagedCommand<DriveCommandConfig> {
dartDefines: config.dartDefines,
);

var exitCode = 0;

_testRunner
..repeats = config.repeat
..builder = (target, device) async {
try {
await _flutterTool.build(target, device);
} catch (err) {
exitCode = 1;
_logger
..err('$err')
..err(
Expand All @@ -242,8 +242,7 @@ class DriveCommand extends StagedCommand<DriveCommandConfig> {
..executor = (target, device) async {
try {
await _flutterTool.drive(target, device);
} on FlutterDriverFailedException catch (err) {
exitCode = 1;
} catch (err) {
_logger
..err('$err')
..err(
Expand Down Expand Up @@ -276,8 +275,29 @@ class DriveCommand extends StagedCommand<DriveCommandConfig> {
}
}

await _testRunner.run();
final results = await _testRunner.run();

for (final res in results.targetRunResults) {
if (res.allRunsPassed) {
_logger.write(
'${' PASS '.bgGreen.black.bold} ${res.targetName} on ${res.device.id}\n',
);
} else if (res.allRunsFailed) {
_logger.write(
'${' FAIL '.bgRed.white.bold} ${res.targetName} on ${res.device.id}\n',
);
} else if (res.canceled) {
_logger.write(
'${' CANC '.bgGray.white.bold} ${res.targetName} on ${res.device.id}\n',
);
} else {
_logger.write(
'${' FLAK '.bgYellow.black.bold} ${res.targetName} on ${res.device.id}\n',
);
}
}

final exitCode = results.allSuccessful ? 0 : 1;
return exitCode;
}
}
76 changes: 75 additions & 1 deletion packages/patrol_cli/lib/src/features/drive/test_runner.dart
Original file line number Diff line number Diff line change
@@ -1,8 +1,65 @@
import 'package:dispose_scope/dispose_scope.dart';
import 'package:equatable/equatable.dart';
import 'package:path/path.dart' show basename;
import 'package:patrol_cli/src/features/drive/device.dart';

typedef _Callback = Future<void> Function(String target, Device device);

class TestRunnerResult with EquatableMixin {
const TestRunnerResult({required this.targetRunResults});

final List<TargetRunResult> targetRunResults;

bool get allSuccessful => targetRunResults
.every((e) => e.runs.every((run) => run == TargetRunStatus.passed));
bartekpacia marked this conversation as resolved.
Show resolved Hide resolved

@override
List<Object?> get props => [targetRunResults];
}

class TargetRunResult with EquatableMixin {
TargetRunResult({
required this.target,
required this.device,
required this.runs,
});

final String target;
final Device device;

final List<TargetRunStatus> runs;

String get targetName => basename(target);

bool get allRunsPassed => runs.every((run) => run == TargetRunStatus.passed);

bool get allRunsFailed => runs.every(
(run) =>
run == TargetRunStatus.failedToBuild ||
run == TargetRunStatus.failedToExecute,
);

/// True if at least 1 test run was canceled.
bool get canceled => runs.any((run) => run == TargetRunStatus.canceled);

int get passedRuns {
return runs.where((run) => run == TargetRunStatus.passed).length;
}

int get runsFailedToBuild {
return runs.where((run) => run == TargetRunStatus.failedToBuild).length;
}

int get runsFailedToExecute {
return runs.where((run) => run == TargetRunStatus.failedToExecute).length;
}

@override
List<Object?> get props => [target, device, runs];
}

enum TargetRunStatus { failedToBuild, failedToExecute, passed, canceled }

/// Orchestrates running tests on devices.
///
/// It maps running T test targets on D devices, resulting in T * D test runs.
Expand Down Expand Up @@ -78,7 +135,7 @@ class TestRunner extends Disposable {
/// Tests are run sequentially on a single device, but many devices can be
/// attached at the same time, and thus many tests can be running at the same
/// time.
Future<void> run() async {
Future<TestRunnerResult> run() async {
if (_running) {
throw StateError('tests are already running');
}
Expand All @@ -101,30 +158,45 @@ class TestRunner extends Disposable {

_running = true;

final targetRunResults = <TargetRunResult>[];

final testRunsOnAllDevices = <Future<void>>[];
for (final device in _devices.values) {
Future<void> runTestsOnDevice() async {
for (final target in _targets) {
final targetRuns = <TargetRunStatus>[];
targetRunResults.add(
TargetRunResult(target: target, device: device, runs: targetRuns),
);
Comment on lines +167 to +169
Copy link
Member

Choose a reason for hiding this comment

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

I know it's probably not that much of an issue here, but having TargetRunResult as an Equatable and then mutating its fields seems like a risky maneuver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? I don't think it breaks Equatable's contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok. Besides the fact that i do not like mutability at all, but whether it is Equatable or not doesn't change anything here.


if (_disposed) {
for (var i = 0; i < _repeats; i++) {
targetRuns.add(TargetRunStatus.canceled);
}
continue;
}

try {
await builder(target, device);
} catch (_) {
targetRuns.add(TargetRunStatus.failedToBuild);
continue;
}

for (var i = 0; i < _repeats; i++) {
if (_disposed) {
targetRuns.add(TargetRunStatus.canceled);
continue;
}

try {
await executor(target, device);
} catch (_) {
targetRuns.add(TargetRunStatus.failedToExecute);
continue;
}

targetRuns.add(TargetRunStatus.passed);
}
}
}
Expand All @@ -135,6 +207,8 @@ class TestRunner extends Disposable {

await Future.wait<void>(testRunsOnAllDevices);
_running = false;

return TestRunnerResult(targetRunResults: targetRunResults);
}

@override
Expand Down
7 changes: 7 additions & 0 deletions packages/patrol_cli/pubspec.lock
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,13 @@ packages:
url: "https://pub.dartlang.org"
source: hosted
version: "2.0.1"
equatable:
dependency: "direct main"
description:
name: equatable
url: "https://pub.dartlang.org"
source: hosted
version: "2.0.5"
fake_async:
dependency: "direct dev"
description:
Expand Down
1 change: 1 addition & 0 deletions packages/patrol_cli/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ dependencies:
args: ^2.3.1
collection: ^1.16.0
dispose_scope: ^2.0.1
equatable: ^2.0.5
file: ^6.1.4
freezed_annotation: ^2.1.0
http: ^0.13.5
Expand Down
Loading