Skip to content

Commit

Permalink
[blinkpy] Convert a check for redundant expectations to warning
Browse files Browse the repository at this point in the history 
Change redundant_virtual_expectations from failures to warnings.
This is a temporary workaround for https://crbug.com/1080691.

Bug: 1080691, 1072015
Change-Id: I87964ecb86aa64faa7590badfa7f5478735cbc53
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2190587
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#766899}
  • Loading branch information
@Hexcles
Hexcles authored and Commit Bot committed May 8, 2020
1 parent 3833139 commit 4ee474c
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 37 deletions.
38 changes: 26 additions & 12 deletions third_party/blink/tools/blinkpy/web_tests/lint_test_expectations.py
View file
Expand Up @@ -56,9 +56,11 @@ def PresubmitCheckTestExpectations(input_api, output_api):
output_api.PresubmitError("lint_test_expectations.py failed "
"to produce output; check by hand. ")
]
if errs.strip() != 'Lint succeeded.':
return [output_api.PresubmitError(errs)]
return []
if errs.strip() == 'Lint succeeded.':
return []
if errs.rstrip().endswith('Lint succeeded with warnings.'):
return [output_api.PresubmitPromptWarning(errs)]
return [output_api.PresubmitError(errs)]


def lint(host, options):
Expand Down Expand Up @@ -90,6 +92,7 @@ def lint(host, options):
# (the default Port for this host) and it would work the same.

failures = []
warnings = []
expectations_dict = {}
all_system_specifiers = set()
all_build_specifiers = set(ports_to_lint[0].ALL_BUILD_TYPES)
Expand Down Expand Up @@ -121,15 +124,16 @@ def lint(host, options):
test_expectations = TestExpectations(
ports_to_lint[0], expectations_dict={path: content})
# Check each expectation for issues
failures.extend(
_check_expectations(host, ports_to_lint[0], path,
test_expectations))
f, w = _check_expectations(host, ports_to_lint[0], path,
test_expectations)
failures += f
warnings += w
except ParseError as error:
_log.error(str(error))
failures.append(str(error))
_log.error('')

return failures
return failures, warnings


def _check_expectations_file_content(content):
Expand Down Expand Up @@ -234,7 +238,8 @@ def _check_redundant_virtual_expectations(host, port, path, expectations):
error = "{}:{} Expectation '{}' is redundant with '{}' in line {}".format(
host.filesystem.basename(path), exp.lineno, exp.test,
base_test, base_exp.lineno)
_log.error(error)
# TODO(crbug.com/1080691): Change to error once it's fixed.
_log.warning(error)
failures.append(error)

return failures
Expand All @@ -246,9 +251,11 @@ def _check_expectations(host, port, path, test_expectations):
expectations = test_expectations.get_updated_lines(path)
failures = _check_existence(host, port, path, expectations)
failures.extend(_check_directory_glob(host, port, path, expectations))
failures.extend(
_check_redundant_virtual_expectations(host, port, path, expectations))
return failures
# TODO(crbug.com/1080691): Change this to failures once
# wpt_expectations_updater is fixed.
warnings = _check_redundant_virtual_expectations(host, port, path,
expectations)
return failures, warnings


def check_virtual_test_suites(host, options):
Expand Down Expand Up @@ -342,7 +349,11 @@ def check_smoke_tests(host, options):

def run_checks(host, options):
failures = []
failures.extend(lint(host, options))
warnings = []

f, w = lint(host, options)
failures += f
warnings += w
failures.extend(check_virtual_test_suites(host, options))
failures.extend(check_smoke_tests(host, options))

Expand All @@ -353,6 +364,9 @@ def run_checks(host, options):
if failures:
_log.error('Lint failed.')
return 1
elif warnings:
_log.warning('Lint succeeded with warnings.')
return 2
else:
_log.info('Lint succeeded.')
return 0
Expand Down
76 changes: 51 additions & 25 deletions third_party/blink/tools/blinkpy/web_tests/lint_test_expectations_unittest.py
View file
Expand Up @@ -109,8 +109,9 @@ def test_all_configurations(self):
'platform': 'a',
'additional_expectations': []
})
res = lint_test_expectations.lint(host, options)
self.assertEqual(res, [])
failures, warnings = lint_test_expectations.lint(host, options)
self.assertEqual(failures, [])
self.assertEqual(warnings, [])
self.assertEqual(host.ports_parsed, ['a', 'b', 'b-win'])

@unittest.skip(
Expand All @@ -124,8 +125,9 @@ def test_lint_test_files(self):

host.port_factory.all_port_names = lambda platform=None: [platform]

res = lint_test_expectations.lint(host, options)
self.assertEqual(res, [])
failures, warnings = lint_test_expectations.lint(host, options)
self.assertEqual(failures, [])
self.assertEqual(warnings, [])

def test_lint_test_files_errors(self):
options = optparse.Values({
Expand All @@ -141,9 +143,10 @@ def test_lint_test_files_errors(self):
host.port_factory.get = lambda platform, options=None: port
host.port_factory.all_port_names = lambda platform=None: [port.name()]

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertTrue(failures)
self.assertEqual(warnings, [])

self.assertTrue(res)
all_logs = ''.join(self.logMessages())
self.assertIn('foo', all_logs)
self.assertIn('bar', all_logs)
Expand All @@ -164,9 +167,10 @@ def test_extra_files_errors(self):
host.filesystem.write_text_file(WEB_TEST_DIR + '/LeakExpectations',
'-- syntax error')

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertTrue(failures)
self.assertEqual(warnings, [])

self.assertTrue(res)
all_logs = ''.join(self.logMessages())
self.assertIn('LeakExpectations', all_logs)

Expand All @@ -184,9 +188,10 @@ def test_lint_flag_specific_expectation_errors(self):
host.port_factory.get = lambda platform, options=None: port
host.port_factory.all_port_names = lambda platform=None: [port.name()]

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertTrue(failures)
self.assertEqual(warnings, [])

self.assertTrue(res)
all_logs = ''.join(self.logMessages())
self.assertIn('flag-specific', all_logs)
self.assertIn('does/not/exist', all_logs)
Expand All @@ -212,9 +217,10 @@ def test_lint_conflicts_in_test_expectations_between_os_and_os_version(
host.port_factory.get = lambda platform, options=None: port
host.port_factory.all_port_names = lambda platform=None: [port.name()]

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertTrue(failures)
self.assertEqual(warnings, [])

self.assertTrue(res)
all_logs = ''.join(self.logMessages())
self.assertIn('conflict', all_logs)

Expand Down Expand Up @@ -260,10 +266,11 @@ def test_lint_exsistence(self):
host.port_factory.get = lambda platform, options=None: port
host.port_factory.all_port_names = lambda platform=None: [port.name()]

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertTrue(failures)
self.assertEqual(warnings, [])

self.assertTrue(res)
self.assertEquals(len(res), 6)
self.assertEquals(len(failures), 6)
expected_non_existence = [
'test1/*',
'test2/bar.html',
Expand All @@ -272,9 +279,9 @@ def test_lint_exsistence(self):
'virtual/bar/*',
'virtual/bar/test2/foo.html',
]
for i in range(len(res)):
self.assertIn('Test does not exist', res[i])
self.assertIn(expected_non_existence[i], res[i])
for i in range(len(failures)):
self.assertIn('Test does not exist', failures[i])
self.assertIn(expected_non_existence[i], failures[i])

def test_lint_globs(self):
options = optparse.Values({
Expand All @@ -297,9 +304,10 @@ def test_lint_globs(self):
host.port_factory.get = lambda platform, options=None: port
host.port_factory.all_port_names = lambda platform=None: [port.name()]

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertTrue(failures)
self.assertEqual(warnings, [])

self.assertTrue(res)
all_logs = ''.join(self.logMessages())
self.assertIn('directory', all_logs)

Expand Down Expand Up @@ -330,10 +338,12 @@ def test_virtual_test_redundant_expectation(self):
host.port_factory.get = lambda platform, options=None: port
host.port_factory.all_port_names = lambda platform=None: [port.name()]

res = lint_test_expectations.lint(host, options)
failures, warnings = lint_test_expectations.lint(host, options)
self.assertEqual(failures, [])
self.assertTrue(warnings)

self.assertEquals(len(res), 1)
self.assertIn('redundant', res[0])
self.assertEquals(len(warnings), 1)
self.assertIn('redundant', warnings[0])


class CheckVirtualSuiteTest(unittest.TestCase):
Expand Down Expand Up @@ -412,13 +422,29 @@ def tearDown(self):
lint_test_expectations.check_virtual_test_suites = self.orig_check_fn

def test_success(self):
lint_test_expectations.lint = lambda host, options: []
lint_test_expectations.lint = lambda host, options: ([], [])
res = lint_test_expectations.main(['--platform', 'test'], self.stderr)
self.assertEqual('Lint succeeded.', self.stderr.getvalue().strip())
self.assertEqual(res, 0)

def test_success_with_warning(self):
lint_test_expectations.lint = lambda host, options: ([],
['test warning'])
res = lint_test_expectations.main(['--platform', 'test'], self.stderr)
self.assertEqual('Lint succeeded with warnings.',
self.stderr.getvalue().strip())
self.assertEqual(res, 2)

def test_failure(self):
lint_test_expectations.lint = lambda host, options: ['test failure']
lint_test_expectations.lint = lambda host, options: (['test failure'],
[])
res = lint_test_expectations.main(['--platform', 'test'], self.stderr)
self.assertEqual('Lint failed.', self.stderr.getvalue().strip())
self.assertEqual(res, 1)

def test_failure_with_warning(self):
lint_test_expectations.lint = lambda host, options: (['test failure'],
['test warning'])
res = lint_test_expectations.main(['--platform', 'test'], self.stderr)
self.assertEqual('Lint failed.', self.stderr.getvalue().strip())
self.assertEqual(res, 1)
Expand Down

0 comments on commit 4ee474c

Please sign in to comment.