Skip to content

Support covdir in Phabricator notifier #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 18, 2019
Merged

Support covdir in Phabricator notifier #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f4b5e1
bot: Convert unit tests to covdir.
Jul 18, 2019
26c3c1a
bot: Convert phabricator coverage access to covdir.
Jul 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion bot/code_coverage_bot/phabricator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
37 changes: 37 additions & 0 deletions bot/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,3 +331,40 @@ def mock_taskcluster():
taskcluster_config.options = {
'rootUrl': 'http://taskcluster.test',
}


def covdir_report(codecov):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good workaround for now, but maybe we should in the future totally remove the mock reports in the old format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but i wanted to show that the data did not change in that diff.

'''
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'),
}
5 changes: 3 additions & 2 deletions bot/tests/test_notifier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)

Expand Down
59 changes: 33 additions & 26 deletions bot/tests/test_phabricator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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: {
Expand All @@ -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

Expand Down Expand Up @@ -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: {}
Expand All @@ -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 == {}

Expand All @@ -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],
Expand All @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {}
Expand All @@ -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: {
Expand Down