Skip to content

Commit a6a48db

Browse files
author
Gaetano Guerriero
committed
[DEV-6455] Do not set first_comment_on_first_review on facts of unreviewed PRs
1 parent f7b9c45 commit a6a48db

File tree

4 files changed

+99
-63
lines changed

4 files changed

+99
-63
lines changed

‎server/athenian/api/internal/miners/github/pull_request.py‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2903,27 +2903,29 @@ def __call__(self, pr: MinedPullRequest) -> PullRequestFacts:
29032903
)
29042904
if closed and first_review_exact and first_review_exact > closed:
29052905
first_review_exact = None
2906-
first_comment_on_first_review = first_review_exact or merged
2907-
if first_comment_on_first_review:
2906+
potential_first_comment_on_first_review = first_review_exact or merged
2907+
if potential_first_comment_on_first_review:
29082908
committed_dates = pr.commits[PullRequestCommit.committed_date.name]
29092909
try:
29102910
last_commit_before_first_review = committed_dates[
2911-
committed_dates <= first_comment_on_first_review
2911+
committed_dates <= potential_first_comment_on_first_review
29122912
].max()
29132913
except ValueError:
29142914
last_commit_before_first_review = None
29152915
if not (last_commit_before_first_review_own := bool(last_commit_before_first_review)):
2916-
last_commit_before_first_review = first_comment_on_first_review
2916+
last_commit_before_first_review = potential_first_comment_on_first_review
29172917
# force pushes that were lost
29182918
first_commit = nonemin(first_commit, last_commit_before_first_review)
29192919
last_commit = nonemax(last_commit, first_commit)
29202920
first_review_request_backup = nonemin(
2921-
nonemax(created, last_commit_before_first_review), first_comment_on_first_review,
2921+
nonemax(created, last_commit_before_first_review),
2922+
potential_first_comment_on_first_review,
29222923
)
29232924
else:
29242925
last_commit_before_first_review = None
29252926
last_commit_before_first_review_own = False
29262927
first_review_request_backup = None
2928+
29272929
first_review_request_exact = pr.review_requests.nonemin(
29282930
PullRequestReviewRequest.created_at.name,
29292931
)
@@ -2934,7 +2936,7 @@ def __call__(self, pr: MinedPullRequest) -> PullRequestFacts:
29342936
if (
29352937
first_review_request_backup
29362938
and first_review_request
2937-
and first_review_request > first_comment_on_first_review
2939+
and first_review_request > potential_first_comment_on_first_review
29382940
):
29392941
# we cannot request a review after we received a review
29402942
first_review_request = first_review_request_backup
@@ -2951,6 +2953,12 @@ def __call__(self, pr: MinedPullRequest) -> PullRequestFacts:
29512953
if first_review_request and not first_review_exact:
29522954
first_review_request = nonemax(first_review_request, last_commit)
29532955

2956+
# only PR with a review will have `first_comment_on_first_review` filled
2957+
if potential_first_comment_on_first_review and first_review_exact:
2958+
first_comment_on_first_review = potential_first_comment_on_first_review
2959+
else:
2960+
first_comment_on_first_review = None
2961+
29542962
if closed:
29552963
if first_review_request and first_review_request > closed:
29562964
first_review_request = None

‎server/tests/controllers/metrics_controller/test_calc_metrics_prs.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,9 @@ class TestCalcMetricsPRs(BaseCalcMetricsPRsTest):
9191
(PullRequestMetricID.PR_REJECTED, 3),
9292
(PullRequestMetricID.PR_CLOSED, 51),
9393
(PullRequestMetricID.PR_DONE, 22),
94-
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, 51),
95-
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT, 51),
96-
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT_Q, 51),
94+
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, 46),
95+
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT, 46),
96+
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT_Q, 46),
9797
(PullRequestMetricID.PR_SIZE, 51),
9898
(PullRequestMetricID.PR_DEPLOYMENT_TIME, 0), # because pdb is empty
9999
],

‎server/tests/controllers/test_histograms_controller.py‎

Lines changed: 19 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -466,17 +466,9 @@ async def test_calc_histogram_prs_ticks(client, headers):
466466

467467
async def test_calc_histogram_prs_groups(client, headers):
468468
body = {
469-
"for": [
470-
{
471-
"repositories": ["{1}"],
472-
"repogroups": [[0], [0]],
473-
},
474-
],
469+
"for": [{"repositories": ["{1}"], "repogroups": [[0], [0]]}],
475470
"histograms": [
476-
{
477-
"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME,
478-
"scale": "log",
479-
},
471+
{"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, "scale": "log"},
480472
],
481473
"date_from": "2017-10-13",
482474
"date_to": "2018-01-23",
@@ -499,36 +491,16 @@ async def test_calc_histogram_prs_groups(client, headers):
499491
"for": {"repositories": ["{1}"]},
500492
"metric": "pr-wait-first-review-time",
501493
"scale": "log",
502-
"ticks": [
503-
"60s",
504-
"184s",
505-
"569s",
506-
"1752s",
507-
"5398s",
508-
"16624s",
509-
"51201s",
510-
"157688s",
511-
"485648s",
512-
],
513-
"frequencies": [6, 5, 8, 5, 4, 14, 10, 2],
514-
"interquartile": {"left": "790s", "right": "45167s"},
494+
"ticks": ["60s", "216s", "783s", "2829s", "10221s", "36927s", "133408s", "481972s"],
495+
"frequencies": [6, 6, 5, 6, 5, 8, 2],
496+
"interquartile": {"left": "661s", "right": "37901s"},
515497
}
516498

517499

518500
async def test_calc_histogram_prs_lines(client, headers):
519501
body = {
520-
"for": [
521-
{
522-
"repositories": ["{1}"],
523-
"lines": [0, 100, 100500],
524-
},
525-
],
526-
"histograms": [
527-
{
528-
"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME,
529-
"scale": "log",
530-
},
531-
],
502+
"for": [{"repositories": ["{1}"], "lines": [0, 100, 100500]}],
503+
"histograms": [{"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, "scale": "log"}],
532504
"date_from": "2017-10-13",
533505
"date_to": "2018-05-23",
534506
"exclude_inactive": False,
@@ -551,27 +523,26 @@ async def test_calc_histogram_prs_lines(client, headers):
551523
"scale": "log",
552524
"ticks": [
553525
"60s",
554-
"195s",
555-
"637s",
556-
"2078s",
557-
"6776s",
558-
"22090s",
559-
"72014s",
560-
"234763s",
561-
"765318s",
526+
"226s",
527+
"856s",
528+
"3237s",
529+
"12234s",
530+
"46234s",
531+
"174714s",
532+
"660223s",
562533
"2494902s",
563534
],
564-
"frequencies": [4, 8, 7, 8, 4, 22, 8, 6, 1],
565-
"interquartile": {"left": "1762s", "right": "62368s"},
535+
"frequencies": [5, 5, 6, 5, 7, 9, 5, 2],
536+
"interquartile": {"left": "1347s", "right": "58215s"},
566537
}
567538

568539
assert body[1] == {
569540
"for": {"repositories": ["{1}"], "lines": [100, 100500]},
570541
"metric": "pr-wait-first-review-time",
571542
"scale": "log",
572543
"ticks": ["60s", "273s", "1243s", "5661s", "25774s", "117343s", "534220s"],
573-
"frequencies": [8, 4, 1, 4, 7, 2],
574-
"interquartile": {"left": "60s", "right": "49999s"},
544+
"frequencies": [8, 4, 1, 3, 6, 2],
545+
"interquartile": {"left": "60s", "right": "44357s"},
575546
}
576547

577548

@@ -650,12 +621,7 @@ async def test_calc_histogram_prs_logical(
650621
"repositories": ["github.com/src-d/go-git/alpha", "github.com/src-d/go-git/beta"],
651622
},
652623
],
653-
"histograms": [
654-
{
655-
"metric": PullRequestMetricID.PR_REVIEW_TIME,
656-
"scale": "log",
657-
},
658-
],
624+
"histograms": [{"metric": PullRequestMetricID.PR_REVIEW_TIME, "scale": "log"}],
659625
"date_from": "2015-10-13",
660626
"date_to": "2020-05-23",
661627
"exclude_inactive": False,

‎server/tests/internal/miners/github/test_pull_request.py‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
from tests.testutils.db import DBCleaner, models_insert
6565
from tests.testutils.factory import metadata as md_factory
6666
from tests.testutils.factory.common import DEFAULT_JIRA_ACCOUNT_ID, DEFAULT_MD_ACCOUNT_ID
67+
from tests.testutils.factory.wizards import insert_repo, pr_models
6768
from tests.testutils.time import dt
6869

6970

@@ -487,7 +488,8 @@ def validate_pull_request_facts(prmeta: Dict[str, Any], prt: PullRequestFacts):
487488
assert prt.first_review_request <= prt.first_comment_on_first_review
488489
else:
489490
assert not prt.last_review
490-
assert not prt.last_commit_before_first_review
491+
if prt.reviews:
492+
assert not prt.last_commit_before_first_review
491493
if prt.first_review_request_exact:
492494
assert prt.first_review_request
493495
if prt.approved:
@@ -726,6 +728,66 @@ async def test_pr_facts_miner_empty_releases(
726728
validate_pull_request_facts(*prt)
727729

728730

731+
@with_defer
732+
async def test_pr_facts_first_comment_first_review_not_reviewd(mdb_rw, pdb, rdb, sdb, pr_miner):
733+
async with DBCleaner(mdb_rw) as mdb_cleaner:
734+
repo = md_factory.RepositoryFactory(node_id=99, full_name="o/r")
735+
await insert_repo(repo, mdb_cleaner, mdb_rw, sdb)
736+
737+
models = [
738+
*pr_models(
739+
99,
740+
1,
741+
1,
742+
repository_full_name="o/r",
743+
created_at=dt(2011, 1, 2),
744+
closed_at=dt(2011, 1, 15),
745+
merged_at=dt(2011, 1, 15),
746+
user_login="user0",
747+
merged_by_login="user0",
748+
),
749+
]
750+
mdb_cleaner.add_models(*models)
751+
await models_insert(mdb_rw, *models)
752+
753+
prefixer = await Prefixer.load((DEFAULT_MD_ACCOUNT_ID,), mdb_rw, None)
754+
settings = Settings.from_account(1, prefixer, sdb, mdb_rw, None, None)
755+
release_settings = await settings.list_release_matches()
756+
branches, default_branches = await BranchMiner.load_branches(
757+
{"o/r"}, prefixer, 1, (DEFAULT_MD_ACCOUNT_ID,), mdb_rw, None, None,
758+
)
759+
760+
miner, _, _, _ = await pr_miner.mine(
761+
date(2011, 1, 1),
762+
date(2012, 1, 1),
763+
dt(2011, 1, 1),
764+
dt(2012, 1, 1),
765+
{"o/r"},
766+
{},
767+
LabelFilter.empty(),
768+
JIRAFilter.empty(),
769+
True,
770+
branches,
771+
default_branches,
772+
False,
773+
release_settings,
774+
LogicalRepositorySettings.empty(),
775+
prefixer,
776+
1,
777+
(DEFAULT_MD_ACCOUNT_ID,),
778+
mdb_rw,
779+
pdb,
780+
rdb,
781+
None,
782+
)
783+
facts_miner = PullRequestFactsMiner({})
784+
prs_facts = [facts_miner(mined_pr) for mined_pr in miner]
785+
pr_facts = prs_facts[0]
786+
assert pr_facts.first_review_request == np.datetime64(datetime(2011, 1, 15))
787+
assert pr_facts.first_review_request_exact is None
788+
assert pr_facts.first_comment_on_first_review is None
789+
790+
729791
@with_defer
730792
async def test_pr_mine_by_ids(
731793
branches,

0 commit comments

Comments
 (0)