[ios][tools] A design issue in xcode_backend.dart resulted in poor testability

Use case

The xcode_backend.dart mainly contains a Context class, and the current design is:

// xcode_backend.dart
class Context {
  void func1() { ... }
  void func2() { ... }
}

// xcode_backend_test.dart (unit tests in general.shard)
class TestContext {
  @override
  void func1() { ... }
  @override
  void func2() { ... }
}

For example, one of such functions is runSync:

flutter/packages/flutter_tools/bin/xcode_backend.dart

Lines 105 to 145 in 5483fef

    
             /// Run given command in a synchronous subprocess. 
        
             /// 
        
             /// Will throw [Exception] if the exit code is not 0. 
        
             ProcessResult runSync( 
        
               String bin, 
        
               List<String> args, { 
        
               bool verbose = false, 
        
               bool allowFail = false, 
        
               String? workingDirectory, 
        
             }) { 
        
               if (verbose) { 
        
                 print('♦ $bin ${args.join(' ')}'); 
        
               } 
        
               final ProcessResult result = Process.runSync(bin, args, workingDirectory: workingDirectory); 
        
               if (verbose) { 
        
                 print((result.stdout as String).trim()); 
        
               } 
        
               final String resultStderr = result.stderr.toString().trim(); 
        
               if (resultStderr.isNotEmpty) { 
        
                 final errorOutput = StringBuffer(); 
        
                 if (result.exitCode != 0) { 
        
                   // "error:" prefix makes this show up as an Xcode compilation error. 
        
                   errorOutput.write('error: '); 
        
                 } 
        
                 errorOutput.write(resultStderr); 
        
                 echoError(errorOutput.toString()); 
        
                 // Stream stderr to the Flutter build process. 
        
                 // When in verbose mode, `echoError` above will show the logs. So only 
        
                 // stream if not in verbose mode to avoid duplicate logs. 
        
                 // Also, only stream if exitCode is 0 since errors are handled separately 
        
                 // by the tool on failure. 
        
                 if (!verbose && exitCode == 0) { 
        
                   streamOutput(errorOutput.toString()); 
        
                 } 
        
               } 
        
               if (!allowFail && result.exitCode != 0) { 
        
                 throw Exception('Command "$bin ${args.join(' ')}" exited with code ${result.exitCode}'); 
        
               } 
        
               return result; 
        
             }

And it's overwritten here:

flutter/packages/flutter_tools/test/general.shard/xcode_backend_test.dart

Lines 1056 to 1069 in 5483fef

    
           @override 
        
           ProcessResult runSync( 
        
             String bin, 
        
             List<String> args, { 
        
             bool verbose = false, 
        
             bool allowFail = false, 
        
             String? workingDirectory, 
        
           }) { 
        
             return processManager.runSync( 
        
               <dynamic>[bin, ...args], 
        
               workingDirectory: workingDirectory, 
        
               environment: environment, 
        
             ); 
        
           }

This means during unit test, the whole chunk of actual implementation is stubbed out, which defeats the whole purpose of unit tests! For unit test, we should stub out the dependencies of the test subject, not the implementation of test subject itself.

Proposal

Quick fix (short term)

Context can instead introduce a 1-line wrapper function that only contains https://github.com/flutter/flutter/blob/5483fef5384821748cfcc1bfc3157e1f9c0322ea/packages/flutter_tools/bin/xcode_backend.dart#L118C34-L118C41

Then TestContext will just override this wrapper function. Then the rest of the implementation would be run during unit test.

Actual fix

The fact that Context is only inherited by TestContext in tests is an indicator that inheritance isn't the best approach here. We should instead favor composition, by injecting dependencies (e.g. ProcessManager).

Another drawback with the current design is described here, with a real bug as an example: #173138

xcode_backend.sh used to be a large bash script containing all this logic. It was migrated exactly to xcode_backend.dart because... bash. Refactoring this script to be more testable is probably not a good use of time. Instead, we could complete the migration and move all the environment variable parsing logic into flutter assemble, and migrate these tests normal flutter_tool testing patterns. We shouldn't invest in a parallel test runner that lives outside of flutter_tool

Refactoring this script to be more testable is probably not a good use of time.

Sounds good

Instead, we could complete the migration and move all the environment variable parsing logic into flutter assemble

Can you provide more context around the migration?

a parallel test runner that lives outside of flutter_tool

what's a parallel test runner outside of flutter tool? (i thought xcode_backend is inside the flutter tool?) could you explain more?

@jmagman

xcode_backend.sh used to be a large bash script containing all this logic. It was migrated exactly to xcode_backend.dart because... bash.

@jmagman maybe im missing something, but this inheritance pattern seems to be introduced while migrating to dart: 2d07436#diff-36104e735d9feacdb7f30b49120513c92a5f9e45d305eddf5825acd17ebe2feaR22-R23

This pattern is not migrated from shell (which isn't OOP)

Instead, ... migrate these tests normal flutter_tool testing patterns

Unless I am mistaken, I think we may be proposing the same thing here - to understand what "normal flutter_tool testing patterns" you are referring to, I looked at a few files, and most of them use composition with regular DI (e.g. injecting a fakeProcessManager), rather than overwriting in subclass.

Refactoring this script to be more testable is probably not a good use of time.

I've been giving this some more thought, and I'm now thinking we should prioritize making it more testable, because the whole reason we have this incident is because of the silly bug in runSync. If we have tested this code path before, we wouldn't have this bug in the first place.

Instead, we could complete the migration and move all the environment variable parsing logic into flutter assemble

I don't think doing DI would conflict with the moving the environment variable parsing logic; they seem to be orthogonal and can be handled independently (the parsing doesn't use any dependencies at all)

On the other hand (for the sake of discussion & learning), let's say we want to migrate a logic that does use some dependencies (e.g. the bonjour logic), isn't it more important to have better tests to reduce the risk of regression? Planning to migrate should be a reason for improving the tests, not a reason against it. So we can't just say "hey let's keep it poorly tested because we plan to migrate it anyways" (this paragraph is for a hypothetical case, not my main point, so feel free to ignore)

...and migrate these tests normal flutter_tool testing patterns. We shouldn't invest in a parallel test runner that lives outside of flutter_tool

Again I could be missing some context here so may need some help understanding. My proposal is to DI and remove the inheritance design pattern. I don't think it's creating a parallel test runner.

Navigation Menu

Repository navigation

[ios][tools] A design issue in xcode_backend.dart resulted in poor testability #173133

Use case

Proposal

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	/// Run given command in a synchronous subprocess.
	///
	/// Will throw [Exception] if the exit code is not 0.
	ProcessResult runSync(
	String bin,
	List<String> args, {
	bool verbose = false,
	bool allowFail = false,
	String? workingDirectory,
	}) {
	if (verbose) {
	print('♦ $bin ${args.join(' ')}');
	}
	final ProcessResult result = Process.runSync(bin, args, workingDirectory: workingDirectory);
	if (verbose) {
	print((result.stdout as String).trim());
	}
	final String resultStderr = result.stderr.toString().trim();
	if (resultStderr.isNotEmpty) {
	final errorOutput = StringBuffer();
	if (result.exitCode != 0) {
	// "error:" prefix makes this show up as an Xcode compilation error.
	errorOutput.write('error: ');
	}
	errorOutput.write(resultStderr);
	echoError(errorOutput.toString());

	// Stream stderr to the Flutter build process.
	// When in verbose mode, `echoError` above will show the logs. So only
	// stream if not in verbose mode to avoid duplicate logs.
	// Also, only stream if exitCode is 0 since errors are handled separately
	// by the tool on failure.
	if (!verbose && exitCode == 0) {
	streamOutput(errorOutput.toString());
	}
	}
	if (!allowFail && result.exitCode != 0) {
	throw Exception('Command "$bin ${args.join(' ')}" exited with code ${result.exitCode}');
	}
	return result;
	}

	@override
	ProcessResult runSync(
	String bin,
	List<String> args, {
	bool verbose = false,
	bool allowFail = false,
	String? workingDirectory,
	}) {
	return processManager.runSync(
	<dynamic>[bin, ...args],
	workingDirectory: workingDirectory,
	environment: environment,
	);
	}

[ios][tools] A design issue in xcode_backend.dart resulted in poor testability #173133

Description

Use case

Proposal

Activity

hellohuanlin commented on Aug 1, 2025

jmagman commented on Aug 7, 2025

hellohuanlin commented on Aug 7, 2025

hellohuanlin commented on Aug 7, 2025

hellohuanlin commented on Aug 7, 2025

hellohuanlin commented on Aug 8, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions