Make the checker only run on valid results.
Invalid results are not counted for merging CLs and should not be counted for analyzing the DEPS.
Bug: 923016
Change-Id: Ib8ba7c6721c81c4ba280a4327291d76d651b48e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/build/+/2468659
Commit-Queue: Gregory Guterman <guterman@google.com>
Reviewed-by: Garrett Beaty <gbeaty@chromium.org>
diff --git a/recipes/recipe_modules/chromium_tests/api.py b/recipes/recipe_modules/chromium_tests/api.py
index 4548e14..6edf395 100644
--- a/recipes/recipe_modules/chromium_tests/api.py
+++ b/recipes/recipe_modules/chromium_tests/api.py
@@ -1307,18 +1307,21 @@
return self.m.file.read_json(
'read command lines', command_lines_file, test_data={})
- def _contains_invalid_results(self, unrecoverable_test_suites):
+ def _get_valid_and_invalid_results(self, unrecoverable_test_suites):
+ valid = []
+ invalid = []
for test_suite in unrecoverable_test_suites:
# Both 'with patch' and 'without patch' must have valid results to
# skip CQ retries.
- valid_results, _ = (test_suite.with_patch_failures_including_retry())
- if not valid_results:
- return True
+ valid_results_with_patch, _ = (
+ test_suite.with_patch_failures_including_retry())
+ if valid_results_with_patch and test_suite.has_valid_results(
+ 'without patch'):
+ valid.append(test_suite)
+ else:
+ invalid.append(test_suite)
- if not test_suite.has_valid_results('without patch'):
- return True
-
- return False
+ return valid, invalid
def deapply_deps(self, bot_update_step):
with self.m.context(cwd=self.m.chromium_checkout.working_dir):
@@ -1428,18 +1431,23 @@
self.m.test_utils.summarize_findit_flakiness(self.m, task.test_suites)
+ # This means the tests passed
if not unrecoverable_test_suites:
return None
+ # This means there was a failure of some sort
if self.m.tryserver.is_tryserver:
- if self.revised_test_targets is not None:
+ valid_suites, invalid_suites = self._get_valid_and_invalid_results(
+ unrecoverable_test_suites)
+ # For DEPS autoroll analysis
+ if self.revised_test_targets is not None and valid_suites:
with self.m.step.nest(
'Analyze DEPS autorolls correctness check') as step_result:
step_result.presentation.step_text = (
'This is an informational step for infra maintainers '
'and shouldn\'t impact the build')
- self.analyze_deps_autorolls_correctness(unrecoverable_test_suites)
- if not self._contains_invalid_results(unrecoverable_test_suites):
+ self.analyze_deps_autorolls_correctness(valid_suites)
+ if not invalid_suites:
self.m.cq.set_do_not_retry_build()
return result_pb2.RawResult(
diff --git a/recipes/recipe_modules/chromium_tests/tests/api/trybot_steps_analyze_deps_autorolls.py b/recipes/recipe_modules/chromium_tests/tests/api/trybot_steps_analyze_deps_autorolls.py
index a6ed322..a2e6db3 100644
--- a/recipes/recipe_modules/chromium_tests/tests/api/trybot_steps_analyze_deps_autorolls.py
+++ b/recipes/recipe_modules/chromium_tests/tests/api/trybot_steps_analyze_deps_autorolls.py
@@ -3,11 +3,12 @@
# found in the LICENSE file.
from recipe_engine import post_process
-from RECIPE_MODULES.build.chromium_tests import try_spec
+from RECIPE_MODULES.build.chromium_tests import bot_db, bot_spec, try_spec
DEPS = [
'chromium',
'chromium_tests',
+ 'chromium_swarming',
'depot_tools/tryserver',
'recipe_engine/json',
'recipe_engine/legacy_annotation',
@@ -15,9 +16,11 @@
'recipe_engine/properties',
'recipe_engine/raw_io',
'recipe_engine/step',
+ 'test_utils',
]
+
def RunSteps(api):
assert api.tryserver.is_tryserver
raw_result = api.chromium_tests.trybot_steps()
@@ -25,34 +28,36 @@
def GenTests(api):
- deps_exclusion_spec = lambda: api.json.output({
- 'base': {
- 'exclusions': ['DEPS']
- },
- 'chromium': {
- 'exclusions': []
- },
- 'fuchsia': {
- 'exclusions': []
- },
- })
- # Apparently something modifies this output, so we create a new one each time
- cl_info = lambda: api.json.output([{
- 'owner': {
- # chromium-autoroller
- '_account_id': 1302611
- },
- 'branch': 'master',
- 'revisions': {
- 'abcd1234': {
- '_number': '1',
- 'commit': {
- 'message': 'Change commit message',
- },
- },
- },
- }])
+ def deps_exclusion_spec():
+ return api.json.output({
+ 'base': {
+ 'exclusions': ['DEPS']
+ },
+ 'chromium': {
+ 'exclusions': []
+ },
+ 'fuchsia': {
+ 'exclusions': []
+ },
+ })
+
+ def cl_info():
+ return api.json.output([{
+ 'owner': {
+ # chromium-autoroller
+ '_account_id': 1302611
+ },
+ 'branch': 'master',
+ 'revisions': {
+ 'abcd1234': {
+ '_number': '1',
+ 'commit': {
+ 'message': 'Change commit message',
+ },
+ },
+ },
+ }])
deps_changes = '''
13>src/third_party/fake_lib/fake_file.h
@@ -60,15 +65,81 @@
14>third_party/fake_lib2/fake_file.cpp
'''
+ def common_props():
+ return sum([
+ api.chromium.try_build(
+ builder_group='tryserver.chromium.test',
+ builder='retry-shards',
+ ),
+ api.chromium_tests.builders(
+ bot_db.BotDatabase.create({
+ 'chromium.test': {
+ 'retry-shards':
+ bot_spec.BotSpec.create(
+ chromium_config='chromium',
+ gclient_config='chromium',
+ ),
+ 'retry-shards-test':
+ bot_spec.BotSpec.create(
+ execution_mode=bot_spec.TEST,
+ parent_buildername='retry-shards',
+ ),
+ },
+ 'tryserver.chromium.unmirrored': {
+ 'unmirrored-chromium-rel':
+ bot_spec.BotSpec.create(
+ chromium_config='chromium',
+ gclient_config='chromium',
+ ),
+ },
+ })),
+ api.chromium_tests.trybots(
+ try_spec.TryDatabase.create({
+ 'tryserver.chromium.test': {
+ 'retry-shards':
+ try_spec.TrySpec.create(
+ retry_failed_shards=True,
+ mirrors=[
+ try_spec.TryMirror.create(
+ builder_group='chromium.test',
+ buildername='retry-shards',
+ tester='retry-shards-test',
+ ),
+ ],
+ ),
+ }
+ })),
+ api.properties(
+ swarm_hashes={'base_unittests': '[dummy hash for base_unittests]'
+ },),
+ api.chromium_tests.read_source_side_spec(
+ 'chromium.test', {
+ 'retry-shards': {
+ 'gtest_tests': [{
+ 'test': 'base_unittests',
+ 'swarming': {
+ 'can_use_on_swarming_builders': True,
+ }
+ }],
+ },
+ }),
+ api.override_step_data(
+ 'base_unittests (with patch)',
+ api.chromium_swarming.canned_summary_output(
+ api.test_utils.canned_gtest_output(False), failure=True)),
+ api.override_step_data(
+ 'base_unittests (retry shards with patch)',
+ api.chromium_swarming.canned_summary_output(
+ api.test_utils.canned_gtest_output(False), failure=True)),
+ api.override_step_data(
+ 'base_unittests (without patch)',
+ api.chromium_swarming.canned_summary_output(
+ api.test_utils.canned_gtest_output(True), failure=False)),
+ ], api.empty_test_data())
+
yield api.test(
'analyze deps checker pass',
- api.chromium.try_build(
- builder_group='tryserver.chromium.linux', builder='linux-rel'),
- api.chromium_tests.read_source_side_spec('chromium.linux', {
- 'Linux Tests': {
- 'gtest_tests': ['base_unittests'],
- },
- }),
+ common_props(),
api.override_step_data('read filter exclusion spec',
deps_exclusion_spec()),
api.override_step_data('gerrit fetch current CL info', cl_info()),
@@ -84,8 +155,6 @@
'compile_targets': ['base_unittests'],
'test_targets': ['base_unittests'],
})),
- api.override_step_data('base_unittests (with patch)',
- api.legacy_annotation.failure_step),
api.post_process(
post_process.StepSuccess,
'Analyze DEPS autorolls correctness check.Analyze DEPS correct'),
@@ -94,13 +163,7 @@
yield api.test(
'analyze deps checker empty affected',
- api.chromium.try_build(
- builder_group='tryserver.chromium.linux', builder='linux-rel'),
- api.chromium_tests.read_source_side_spec('chromium.linux', {
- 'Linux Tests': {
- 'gtest_tests': ['base_unittests'],
- },
- }),
+ common_props(),
api.override_step_data('read filter exclusion spec',
deps_exclusion_spec()),
api.override_step_data('gerrit fetch current CL info', cl_info()),
@@ -109,21 +172,13 @@
api.override_step_data(
'Analyze DEPS autorolls.gclient recursively git diff all DEPS',
api.raw_io.stream_output('')),
- api.override_step_data('base_unittests (with patch)',
- api.legacy_annotation.failure_step),
api.post_process(post_process.StepSuccess, 'Analyze DEPS autorolls.Skip'),
api.post_process(post_process.DropExpectation),
)
yield api.test(
'analyze deps checker empty analyze',
- api.chromium.try_build(
- builder_group='tryserver.chromium.linux', builder='linux-rel'),
- api.chromium_tests.read_source_side_spec('chromium.linux', {
- 'Linux Tests': {
- 'gtest_tests': ['base_unittests'],
- },
- }),
+ common_props(),
api.override_step_data('read filter exclusion spec',
deps_exclusion_spec()),
api.override_step_data('gerrit fetch current CL info', cl_info()),
@@ -139,8 +194,6 @@
'compile_targets': [],
'test_targets': [],
})),
- api.override_step_data('base_unittests (with patch)',
- api.legacy_annotation.failure_step),
api.post_process(
post_process.StepSuccess,
'Analyze DEPS autorolls correctness check.Analyze DEPS miss'),
@@ -150,16 +203,10 @@
# make it run some suite other than the one that fails
yield api.test(
'analyze deps checker fail',
- api.chromium.try_build(
- builder_group='tryserver.chromium.linux', builder='linux-rel'),
+ common_props(),
api.override_step_data('read filter exclusion spec',
deps_exclusion_spec()),
api.override_step_data('gerrit fetch current CL info', cl_info()),
- api.chromium_tests.read_source_side_spec('chromium.linux', {
- 'Linux Tests': {
- 'gtest_tests': ['base_unittests'],
- },
- }),
api.override_step_data('git diff to analyze patch',
api.raw_io.stream_output('DEPS')),
api.override_step_data(
@@ -172,8 +219,6 @@
'compile_targets': ['not_base_unittests'],
'test_targets': ['not_base_unittests'],
})),
- api.override_step_data('base_unittests (with patch)',
- api.legacy_annotation.failure_step),
api.post_process(
post_process.StepSuccess,
'Analyze DEPS autorolls correctness check.Analyze DEPS miss'),
@@ -182,16 +227,12 @@
yield api.test(
'analyze deps checker infra fail',
- api.chromium.try_build(
- builder_group='tryserver.chromium.linux', builder='linux-rel'),
+ common_props(),
+ api.override_step_data('read filter exclusion spec',
+ deps_exclusion_spec()),
api.override_step_data('read filter exclusion spec',
deps_exclusion_spec()),
api.override_step_data('gerrit fetch current CL info', cl_info()),
- api.chromium_tests.read_source_side_spec('chromium.linux', {
- 'Linux Tests': {
- 'gtest_tests': ['base_unittests'],
- },
- }),
api.override_step_data('git diff to analyze patch',
api.raw_io.stream_output('DEPS')),
api.override_step_data(
@@ -202,3 +243,23 @@
'Analyze DEPS autorolls.error'),
api.post_process(post_process.DropExpectation),
)
+
+ yield api.test(
+ 'analyze deps checker invalid results',
+ common_props(),
+ api.override_step_data('read filter exclusion spec',
+ deps_exclusion_spec()),
+ api.override_step_data('gerrit fetch current CL info', cl_info()),
+ api.override_step_data('git diff to analyze patch',
+ api.raw_io.stream_output('DEPS')),
+ api.override_step_data(
+ 'Analyze DEPS autorolls.gclient recursively git diff all DEPS',
+ api.raw_io.stream_output(deps_changes)),
+ api.override_step_data(
+ 'base_unittests (without patch)',
+ api.chromium_swarming.canned_summary_output(
+ api.test_utils.canned_gtest_output(False), failure=True)),
+ api.post_process(post_process.DoesNotRun,
+ 'Analyze DEPS autorolls correctness check'),
+ api.post_process(post_process.DropExpectation),
+ )
