diff --git a/bot/code_coverage_bot/phabricator.py b/bot/code_coverage_bot/phabricator.py index 5b383de97..7e3346076 100644 --- a/bot/code_coverage_bot/phabricator.py +++ b/bot/code_coverage_bot/phabricator.py @@ -28,7 +28,19 @@ def __init__(self, repo_dir, revision): self.revision = revision def _find_coverage(self, report, path): - return next((sf['coverage'] for sf in report['source_files'] if sf['name'] == path), None) + ''' + Find coverage value in a covdir report + ''' + assert isinstance(report, dict) + + parts = path.split('/') + for part in filter(None, parts): + if part not in report['children']: + logger.warn('Path {} not found in report'.format(path)) + return + report = report['children'][part] + + return report['coverage'] def _build_coverage_map(self, annotate, coverage_record): # We can't use plain line numbers to map coverage data from the build changeset to the diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 642fa8938..901776e77 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -331,3 +331,40 @@ def mock_taskcluster(): taskcluster_config.options = { 'rootUrl': 'http://taskcluster.test', } + + +def covdir_report(codecov): + ''' + Convert source files to covdir format + ''' + assert isinstance(codecov, dict) + assert 'source_files' in codecov + + out = {} + for cov in codecov['source_files']: + assert '/' not in cov['name'] + coverage = cov['coverage'] + total = len(coverage) + covered = sum(l is not None and l > 0 for l in coverage) + out[cov['name']] = { + 'children': {}, + 'name': cov['name'], + 'coverage': coverage, + 'coveragePercent': 100.0 * covered / total, + 'linesCovered': covered, + 'linesMissed': total - covered, + 'linesTotal': total, + } + + # Covdir has a root level + def _sum(name): + return sum(c[name] for c in out.values()) + return { + 'children': out, + 'name': 'src', + 'coverage': [], + 'coveragePercent': _sum('coveragePercent') / len(out) if out else 0, + 'linesCovered': _sum('linesCovered'), + 'linesMissed': _sum('linesMissed'), + 'linesTotal': _sum('linesTotal'), + } diff --git a/bot/tests/test_notifier.py b/bot/tests/test_notifier.py index be119e7db..43fa06de1 100644 --- a/bot/tests/test_notifier.py +++ b/bot/tests/test_notifier.py @@ -3,6 +3,7 @@ from code_coverage_bot.notifier import notify_email from code_coverage_bot.phabricator import PhabricatorUploader +from conftest import covdir_report from mercurial import add_file from mercurial import changesets from mercurial import commit @@ -28,12 +29,12 @@ def test_notification(mock_secrets, mock_taskcluster, mock_phabricator, fake_hg_ assert stack[0]['desc'] == "Commit [(b'A', b'file')]Differential Revision: https://phabricator.services.mozilla.com/D1" assert stack[1]['desc'] == "Commit [(b'M', b'file')]Differential Revision: https://phabricator.services.mozilla.com/D2" - report = { + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [None, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0], }] - } + }) phab = PhabricatorUploader(local, revision) changesets_coverage = phab.generate(report, stack) diff --git a/bot/tests/test_phabricator.py b/bot/tests/test_phabricator.py index a178f4f86..710b8515b 100644 --- a/bot/tests/test_phabricator.py +++ b/bot/tests/test_phabricator.py @@ -7,6 +7,7 @@ import responses from code_coverage_bot.phabricator import PhabricatorUploader +from conftest import covdir_report from mercurial import add_file from mercurial import changesets from mercurial import commit @@ -25,12 +26,13 @@ def test_simple(mock_secrets, mock_phabricator, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [None, 0, 1, 1, 1, 1, 0], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: { @@ -42,12 +44,7 @@ def test_simple(mock_secrets, mock_phabricator, fake_hg_repo): } } - phabricator.upload({ - 'source_files': [{ - 'name': 'file', - 'coverage': [None, 0, 1, 1, 1, 1, 0], - }] - }, changesets(local, revision)) + phabricator.upload(report, changesets(local, revision)) assert len(responses.calls) >= 3 @@ -97,9 +94,10 @@ def test_file_with_no_coverage(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: {} @@ -118,12 +116,13 @@ def test_one_commit_without_differential(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file_one_commit', 'coverage': [None, 0, 1, 1, 1, 1, 0], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == {} @@ -144,7 +143,7 @@ def test_two_commits_two_files(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file1_commit1', 'coverage': [None, 0, 1, 1, 1, 1, 0], @@ -155,7 +154,8 @@ def test_two_commits_two_files(mock_secrets, fake_hg_repo): 'name': 'file3_commit2', 'coverage': [1, 1, 0, 1, None], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: { @@ -196,12 +196,13 @@ def test_changesets_overwriting(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [None, 0, 1, 1, 1, 1, 0], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: { @@ -236,12 +237,13 @@ def test_changesets_displacing(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [0, 1, None, 0, 1, 1, 1, 1, 0, 1, 0], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: { @@ -276,12 +278,13 @@ def test_changesets_reducing_size(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [None, 0, 1, 1, 1], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: { @@ -316,12 +319,14 @@ def test_changesets_overwriting_one_commit_without_differential(mock_secrets, fa copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [None, 0, 1, 1, 1, 1, 0], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: { @@ -349,9 +354,10 @@ def test_removed_file(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: {} @@ -376,12 +382,13 @@ def test_backout_removed_file(mock_secrets, fake_hg_repo): copy_pushlog_database(remote, local) phabricator = PhabricatorUploader(local, revision) - results = phabricator.generate({ + report = covdir_report({ 'source_files': [{ 'name': 'file', 'coverage': [None, 0, 1, 1, 1, 1, 0], }] - }, changesets(local, revision)) + }) + results = phabricator.generate(report, changesets(local, revision)) assert results == { 1: {