Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions lib/build_verify.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ const defaultCommand = [
/// with the full, platform-specific path to the corresponding executable in the
/// currently executing SDK.
///
/// If [afterBuildCommand] is specified, it will be run after the build and
/// has the same behavior as [customCommand].
///
/// If provided, [gitDiffPathArguments] are passed as `-- <path>` to `git diff`.
/// This can be useful if you want to include certain files from the diff
/// calculation.
Expand All @@ -29,10 +32,12 @@ const defaultCommand = [
Future<void> expectBuildClean({
String? packageRelativeDirectory,
List<String> customCommand = defaultCommand,
List<String>? afterBuildCommand,
List<String>? gitDiffPathArguments,
}) => expectBuildCleanImpl(
Directory.current.resolveSymbolicLinksSync(),
command: customCommand,
afterBuildCommand: afterBuildCommand,
packageRelativeDirectory: packageRelativeDirectory,
gitDiffPathArguments: gitDiffPathArguments,
);
15 changes: 14 additions & 1 deletion lib/src/impl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const dartPlaceHolder = 'DART';
Future<void> expectBuildCleanImpl(
String workingDir, {
List<String> command = defaultCommand,
List<String>? afterBuildCommand,
String? packageRelativeDirectory,
List<String>? gitDiffPathArguments,
}) async {
Expand Down Expand Up @@ -54,7 +55,19 @@ Future<void> expectBuildCleanImpl(

expectResultOutputSucceeds(result);

// 3 - get a list of modified files after the build - should still be empty
// 3 - run after build command
if (afterBuildCommand != null && afterBuildCommand.isNotEmpty) {
var executable = afterBuildCommand.first;
if (executable == 'DART') {
executable = dartPath;
}

final arguments = afterBuildCommand.skip(1).toList();

await _runProc(executable, arguments, workingDir);
}
Comment on lines +59 to +68
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The logic to parse the command, resolve the DART placeholder, and prepare arguments is duplicated from lines 46-52 where the main command is processed. To improve maintainability and avoid redundancy, consider extracting this logic into a private helper function. This function could take a List<String> command and either run the process directly or return the prepared executable and arguments.


// 4 - get a list of modified files after the build - should still be empty
expect(
await _changedGeneratedFiles(
workingDir,
Expand Down
68 changes: 68 additions & 0 deletions test/integration/simple_integration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,72 @@ void main() {
);
});
});

group('afterBuildCommand without file changes', () {
test(
'should succeed if afterBuildCommand does not produce git changes',
() async {
final afterBuildCommand = [dartPlaceHolder, 'pub', 'get'];
await expectBuildCleanImpl(
d.sandbox,
afterBuildCommand: afterBuildCommand,
);
},
);

test(
'should fail if afterBuildCommand itself fails (e.g. command not exists)',
() async {
final failingAfterBuildCommand = ['this_command_does_not_exist'];

await expectLater(
expectBuildCleanImpl(
d.sandbox,
afterBuildCommand: failingAfterBuildCommand,
),
throwsA(isA<ProcessException>()),
);
},
);
});

group('afterBuildCommand when a file changes', () {
const pubspecLockFileName = 'pubspec.lock';

setUp(() async {
await gitDir.runCommand(['add', pubspecLockFileName]);
await gitDir.runCommand(['commit', '-am', 'commit pubspec']);
});

test('should fail if afterBuildCommand produces git changes', () async {
final afterBuildCommand = [
'sh',
'-c',
'echo "new content just for this test" > $pubspecLockFileName',
];

await expectLater(
expectBuildCleanImpl(d.sandbox, afterBuildCommand: afterBuildCommand),
throwsATestFailure,
);
});

test(
'should succeed if afterBuildCommand produces git changes which are then '
'ignored by gitDiffPathArguments',
() async {
final afterBuildCommand = [
'sh',
'-c',
'echo "new content just for this test" > $pubspecLockFileName',
];

await expectBuildCleanImpl(
d.sandbox,
afterBuildCommand: afterBuildCommand,
gitDiffPathArguments: [':!$pubspecLockFileName'],
);
},
);
});
Comment on lines +93 to +131
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The afterBuildCommand variable is defined in both tests within this group, leading to code duplication. To improve readability and maintainability, you can define it once at the group level and reuse it in both tests.

  group('afterBuildCommand when a file changes', () {
    const pubspecLockFileName = 'pubspec.lock';
    final afterBuildCommand = [
      'sh',
      '-c',
      'echo "new content just for this test" > $pubspecLockFileName',
    ];

    setUp(() async {
      await gitDir.runCommand(['add', pubspecLockFileName]);
      await gitDir.runCommand(['commit', '-am', 'commit pubspec']);
    });

    test('should fail if afterBuildCommand produces git changes', () async {
      await expectLater(
        expectBuildCleanImpl(d.sandbox, afterBuildCommand: afterBuildCommand),
        throwsATestFailure,
      );
    });

    test(
      'should succeed if afterBuildCommand produces git changes which are then '
      'ignored by gitDiffPathArguments',
      () async {
        await expectBuildCleanImpl(
          d.sandbox,
          afterBuildCommand: afterBuildCommand,
          gitDiffPathArguments: [':!$pubspecLockFileName'],
        );
      },
    );
  });

}
Loading