Skip to content

Commit

Permalink
Fix race condition causing assertion failure during baseline optimiza…
Browse files Browse the repository at this point in the history 
…tion in rebaseline-cl.

1. Serialize the calls to baseline optimizer with all the tests in one command.

2. Separate optimize commands for each flag-spec version.

Bug: 1304918

Change-Id: I45d0ea1c0e77170446aec2a6e438f12ed559ebaa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3813636
Commit-Queue: Preethi Mohan <preethim@google.com>
Reviewed-by: Weizhong Xia <weizhong@google.com>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033768}
  • Loading branch information
@preethimm
preethimm authored and Chromium LUCI CQ committed Aug 10, 2022
1 parent 883ca75 commit ac30354
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 33 deletions.
41 changes: 31 additions & 10 deletions third_party/blink/tools/blinkpy/tool/commands/rebaseline.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -537,12 +537,24 @@ def _optimize_commands(self,
test_flag_pairs_to_suffixes[test, flag_spec].update(
self._suffixes_for_actual_failures(test, build, step_name))

optimize_commands = []
suffixes_flag_spec = collections.defaultdict(set)
test_list = collections.defaultdict(list)
for (test, flag_spec), suffixes in test_flag_pairs_to_suffixes.items():
# No need to optimize baselines for a test with no failures.
if not suffixes:
continue
path_to_blink_tool = self._tool.path()
if flag_spec is None:
flag_spec = 'default'

suffixes_flag_spec[flag_spec].update(suffixes)
test_list[flag_spec].append(test)

optimize_commands = collections.defaultdict(lambda: ([], ''))
cwd = self._tool.git().checkout_root
path_to_blink_tool = self._tool.path()

# Build one optimize-baselines invocation command for each flag_spec.
# All the tests in the test list will be optimized iteratively.
for flag_spec, test_list_flag in test_list.items():
command = [
self._tool.executable,
path_to_blink_tool,
Expand All @@ -554,11 +566,15 @@ def _optimize_commands(self,
]
if verbose:
command.append('--verbose')
if flag_spec:
if flag_spec != 'default':
command.extend(['--flag-specific', flag_spec])
command.extend(['--suffixes', ','.join(sorted(suffixes)), test])
cwd = self._tool.git().checkout_root
optimize_commands.append((command, cwd))
command.extend([
'--suffixes', ','.join(sorted(suffixes_flag_spec[flag_spec]))
])
for test in test_list_flag:
command.append(test)

optimize_commands[flag_spec] = (command, cwd)

return optimize_commands

Expand Down Expand Up @@ -652,9 +668,14 @@ def rebaseline(self, options, test_baseline_set):
self._update_expectations_files(lines_to_remove)

if options.optimize:
self._run_in_parallel(
self._optimize_commands(test_baseline_set, options.verbose,
options.resultDB))
optimize_commands = self._optimize_commands(
test_baseline_set, options.verbose, options.resultDB)

for _, (cmd, cwd) in optimize_commands.items():
output = self._tool.executive.run_command(cmd, cwd)
# log stderr if any.
for line in output[1]:
print(line)

self._tool.git().add_list(self.unstaged_baselines())

Expand Down
33 changes: 16 additions & 17 deletions third_party/blink/tools/blinkpy/tool/commands/rebaseline_cl_unittest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,15 +649,15 @@ def test_rebaseline_command_invocations(self):
'--step-name',
'blink_web_tests (with patch)',
]],
[[
[
'python',
'echo',
'optimize-baselines',
'--no-manifest-update',
'--suffixes',
'wav',
'one/flaky-fail.html',
]]])
]])

def test_rebaseline_command_invocations_multiple_steps(self):
"""Test the rebaseline tool handles multiple steps on the same builder.
Expand Down Expand Up @@ -700,21 +700,20 @@ def test_rebaseline_command_invocations_multiple_steps(self):
'not_site_per_process_blink_web_tests (with patch)'
],
])
self.assertEqual(sorted(self.tool.executive.calls[2]), [
[
'python', 'echo', 'optimize-baselines', '--no-manifest-update',
'--flag-specific', 'disable-layout-ng', '--suffixes', 'txt',
'one/text-fail.html'
],
[
'python', 'echo', 'optimize-baselines', '--no-manifest-update',
'--flag-specific', 'disable-site-isolation-trials',
'--suffixes', 'txt', 'one/text-fail.html'
],
[
'python', 'echo', 'optimize-baselines', '--no-manifest-update',
'--suffixes', 'txt', 'one/text-fail.html'
]
print(self.tool.executive.calls)
self.assertEqual(self.tool.executive.calls[2], [
'python', 'echo', 'optimize-baselines', '--no-manifest-update',
'--suffixes', 'txt', 'one/text-fail.html'
])
self.assertEqual(self.tool.executive.calls[3], [
'python', 'echo', 'optimize-baselines', '--no-manifest-update',
'--flag-specific', 'disable-layout-ng', '--suffixes', 'txt',
'one/text-fail.html'
])
self.assertEqual(self.tool.executive.calls[4], [
'python', 'echo', 'optimize-baselines', '--no-manifest-update',
'--flag-specific', 'disable-site-isolation-trials', '--suffixes',
'txt', 'one/text-fail.html'
])

def test_trigger_try_jobs(self):
Expand Down
12 changes: 6 additions & 6 deletions third_party/blink/tools/blinkpy/tool/commands/rebaseline_unittest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ def test_rebaseline_all(self):
'--step-name',
'blink_web_tests (with patch)',
]],
[[
[
'python',
'echo',
'optimize-baselines',
Expand All @@ -466,7 +466,7 @@ def test_rebaseline_all(self):
'--suffixes',
'png,txt',
'userscripts/first-test.html',
]]])
]])

def test_rebaseline_debug(self):
test_baseline_set = TestBaselineSet(self.tool)
Expand Down Expand Up @@ -504,7 +504,7 @@ def test_rebaseline_debug(self):
'--step-name',
'blink_web_tests (with patch)',
]],
[[
[
'python',
'echo',
'optimize-baselines',
Expand All @@ -513,7 +513,7 @@ def test_rebaseline_debug(self):
'--suffixes',
'png,txt',
'userscripts/first-test.html',
]]])
]])

def test_no_optimize(self):
test_baseline_set = TestBaselineSet(self.tool)
Expand Down Expand Up @@ -631,7 +631,7 @@ def test_rebaseline_with_different_port_name(self):
'--step-name',
'blink_web_tests (with patch)',
]],
[[
[
'python',
'echo',
'optimize-baselines',
Expand All @@ -640,7 +640,7 @@ def test_rebaseline_with_different_port_name(self):
'--suffixes',
'png,txt',
'userscripts/first-test.html',
]]])
]])


class TestRebaselineUpdatesExpectationsFiles(BaseTestCase):
Expand Down

0 comments on commit ac30354

Please sign in to comment.