I think the method has a race condition. startProcess should return Future<io.Process> and here we should await for the process to be started. Otherwise, there's a chance that _runDriver returns without the process.

This is the implementation of the method. I thought it would await for the process and will add it to the cleanup list.

We are using this method since March: https://github.com/flutter/engine/pull/17190/files#diff-9e0e5a53a06c2f6ab3e0904639468ab6R110

final io.Process process = await io.Process.start(
    executable,
    arguments,
    workingDirectory: workingDirectory,
    // Running the process in a system shell for Windows. Otherwise
    // the process is not able to get Dart from path.
    runInShell: io.Platform.isWindows,
    mode: io.ProcessStartMode.inheritStdio,
  );
  processesToCleanUp.add(process);

Please let me know if I'm missing something.

I'm not talking about clean-up. I'm talking about start-up race condition. The _startDriver method returns before the process is started because it swallows the Future returned by startProcess.

I sent a conversation offline. Let's discuss if there is leak in the process. I can send another fix since the methods in utils.dart are used by other places. I can fix them all in one PR.

As discussed the change was running without the Future (await'in on void) due to our analyze options: https://github.com/flutter/engine/blob/master/lib/web_ui/analysis_options.yaml#L30

Why not have the test file itself specify this, e.g. in a specially formatted comment? It's trivial to parse it out of the test's source code. This file is too removed from the test code to be an intuitive location for this list.

Previously we discussed this. The consensus was to use the blacklists this way and not relying on the contents of the test files.

@mdebbar I think you mentioned that you also don't like the content/name checking on tests. Can you provide more feedback?

I don't have strong opinion. I lean slightly towards this approach, unless we have a lint checker that will detect validity of the comment. I don't like projects inventing their own programing languages/metatags.

Well, a map that follows a particular structure and that contains strings that must follow certain format is already a language, even if it is embedded inside Dart. The issue is that in mere couple of hours after reviewing this PR I already forgot which file this list was defined in :)

I imagine most team members won't even know that this list exists. Typically, I look at other test code to learn how things are done, but that won't help me here because the blacklist is quite detached from the test code.

My issue was with whitelist vs blacklist. I prefer blacklist because I want all new tests to be running everywhere by default.

I agree with Yegor that this is not an intuitive place for the blacklist because when I'm looking into a test, it's not obvious to me where the blacklist lives. That said, I also don't like having a pre-processing step that parses comments for specially formatted configuration.

I prefer solutions like the skip: argument in package:test. Can we have our own test function that takes an extra argument?

import '../our_test.dart';

test('my test', () {
  // Test goes here ...
}, skipOn: ['chrome-linux', 'safari-macos', 'chrome-macos']);

It can be defined like:

import 'package:test/test.dart' as package_test;

typedef TestBody = ...;

void test(String name, TestBody body, {List<String> skipOn}) {
  final bool skip = skipOn.contains(_currentPlatformBrowser);
  return package_test.test(name, body, skip: skip);
}

I was also talking to @yjbanov about luci.dart meanwhile. It looks like this would be the solution after luci.dart: yjbanov/luci.dart#3
Each integration test, will be added as a target. If we want them to run only on a MacOS bot with Safari we will setup the agent and browser.

To sum up the discussion this is summary I collected from our comments:
Test skip: (pros) easy to read. easy to use by the test developer. already existing syntax (cons) more time to add to the test harness.
comments: (cons) pre-processing step that may lead to other errors. (pros) Easy to add to the text harness. visible to the developer.
current suggestion: (cons)very easy to forget (pros) easy to add to the text harness.

I'll continue with this approach. I'll try implement either @yjbanov's or @mdebbar's suggestion if luci.dart is delayed/cancelled, otherwise it will solve this issue.

I would not yet count on luci.dart happening. I still need to sell parts of the team on it. For now, I would encourage us to do the most idiomatic solution within the current system. Having said that, my comment is not blocking. I feel like this will be a source paper cuts in the future, but nothing fatal.