Skip to content

Commit

Permalink
[DEV-6455] Do not set first_comment_on_first_review on facts of unrev…
Browse files Browse the repository at this point in the history 
…iewed PRs
  • Loading branch information
Gaetano Guerriero committed Apr 20, 2023
1 parent f7b9c45 commit 506d785
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 62 deletions.
20 changes: 14 additions & 6 deletions server/athenian/api/internal/miners/github/pull_request.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2903,27 +2903,29 @@ def __call__(self, pr: MinedPullRequest) -> PullRequestFacts:
)
if closed and first_review_exact and first_review_exact > closed:
first_review_exact = None
first_comment_on_first_review = first_review_exact or merged
if first_comment_on_first_review:
potential_first_comment_on_first_review = first_review_exact or merged
if potential_first_comment_on_first_review:
committed_dates = pr.commits[PullRequestCommit.committed_date.name]
try:
last_commit_before_first_review = committed_dates[
committed_dates <= first_comment_on_first_review
committed_dates <= potential_first_comment_on_first_review
].max()
except ValueError:
last_commit_before_first_review = None
if not (last_commit_before_first_review_own := bool(last_commit_before_first_review)):
last_commit_before_first_review = first_comment_on_first_review
last_commit_before_first_review = potential_first_comment_on_first_review
# force pushes that were lost
first_commit = nonemin(first_commit, last_commit_before_first_review)
last_commit = nonemax(last_commit, first_commit)
first_review_request_backup = nonemin(
nonemax(created, last_commit_before_first_review), first_comment_on_first_review,
nonemax(created, last_commit_before_first_review),
potential_first_comment_on_first_review,
)
else:
last_commit_before_first_review = None
last_commit_before_first_review_own = False
first_review_request_backup = None

first_review_request_exact = pr.review_requests.nonemin(
PullRequestReviewRequest.created_at.name,
)
Expand All @@ -2934,7 +2936,7 @@ def __call__(self, pr: MinedPullRequest) -> PullRequestFacts:
if (
first_review_request_backup
and first_review_request
and first_review_request > first_comment_on_first_review
and first_review_request > potential_first_comment_on_first_review
):
# we cannot request a review after we received a review
first_review_request = first_review_request_backup
Expand All @@ -2951,6 +2953,12 @@ def __call__(self, pr: MinedPullRequest) -> PullRequestFacts:
if first_review_request and not first_review_exact:
first_review_request = nonemax(first_review_request, last_commit)

# only PR with a review will have `first_comment_on_first_review` filled
if potential_first_comment_on_first_review and first_review_exact:
first_comment_on_first_review = potential_first_comment_on_first_review
else:
first_comment_on_first_review = None

if closed:
if first_review_request and first_review_request > closed:
first_review_request = None
Expand Down
6 changes: 3 additions & 3 deletions server/tests/controllers/metrics_controller/test_calc_metrics_prs.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,9 @@ class TestCalcMetricsPRs(BaseCalcMetricsPRsTest):
(PullRequestMetricID.PR_REJECTED, 3),
(PullRequestMetricID.PR_CLOSED, 51),
(PullRequestMetricID.PR_DONE, 22),
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, 51),
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT, 51),
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT_Q, 51),
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, 46),
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT, 46),
(PullRequestMetricID.PR_WAIT_FIRST_REVIEW_COUNT_Q, 46),
(PullRequestMetricID.PR_SIZE, 51),
(PullRequestMetricID.PR_DEPLOYMENT_TIME, 0), # because pdb is empty
],
Expand Down
65 changes: 13 additions & 52 deletions server/tests/controllers/test_histograms_controller.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,17 +466,9 @@ async def test_calc_histogram_prs_ticks(client, headers):

async def test_calc_histogram_prs_groups(client, headers):
body = {
"for": [
{
"repositories": ["{1}"],
"repogroups": [[0], [0]],
},
],
"for": [{"repositories": ["{1}"], "repogroups": [[0], [0]]}],
"histograms": [
{
"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME,
"scale": "log",
},
{"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, "scale": "log"},
],
"date_from": "2017-10-13",
"date_to": "2018-01-23",
Expand All @@ -499,36 +491,16 @@ async def test_calc_histogram_prs_groups(client, headers):
"for": {"repositories": ["{1}"]},
"metric": "pr-wait-first-review-time",
"scale": "log",
"ticks": [
"60s",
"184s",
"569s",
"1752s",
"5398s",
"16624s",
"51201s",
"157688s",
"485648s",
],
"frequencies": [6, 5, 8, 5, 4, 14, 10, 2],
"interquartile": {"left": "790s", "right": "45167s"},
"ticks": ["60s", "216s", "783s", "2829s", "10221s", "36927s", "133408s", "481972s"],
"frequencies": [6, 6, 5, 6, 5, 8, 2],
"interquartile": {"left": "661s", "right": "37901s"},
}


async def test_calc_histogram_prs_lines(client, headers):
body = {
"for": [
{
"repositories": ["{1}"],
"lines": [0, 100, 100500],
},
],
"histograms": [
{
"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME,
"scale": "log",
},
],
"for": [{"repositories": ["{1}"], "lines": [0, 100, 100500]}],
"histograms": [{"metric": PullRequestMetricID.PR_WAIT_FIRST_REVIEW_TIME, "scale": "log"}],
"date_from": "2017-10-13",
"date_to": "2018-05-23",
"exclude_inactive": False,
Expand All @@ -549,29 +521,18 @@ async def test_calc_histogram_prs_lines(client, headers):
"for": {"repositories": ["{1}"], "lines": [0, 100]},
"metric": "pr-wait-first-review-time",
"scale": "log",
"ticks": [
"60s",
"195s",
"637s",
"2078s",
"6776s",
"22090s",
"72014s",
"234763s",
"765318s",
"2494902s",
],
"frequencies": [4, 8, 7, 8, 4, 22, 8, 6, 1],
"interquartile": {"left": "1762s", "right": "62368s"},
"ticks": ["201s", "735s", "2690s", "9842s", "36009s", "131739s", "481972s"],
"frequencies": [3, 1, 3, 2, 6, 2],
"interquartile": {"left": "3005s", "right": "55860s"},
}

assert body[1] == {
"for": {"repositories": ["{1}"], "lines": [100, 100500]},
"metric": "pr-wait-first-review-time",
"scale": "log",
"ticks": ["60s", "273s", "1243s", "5661s", "25774s", "117343s", "534220s"],
"frequencies": [8, 4, 1, 4, 7, 2],
"interquartile": {"left": "60s", "right": "49999s"},
"ticks": ["862s", "3089s", "11060s", "39593s", "141742s", "507425s"],
"frequencies": [3, 1, 1, 4, 1],
"interquartile": {"left": "3569s", "right": "56432s"},
}


Expand Down
64 changes: 63 additions & 1 deletion server/tests/internal/miners/github/test_pull_request.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
from tests.testutils.db import DBCleaner, models_insert
from tests.testutils.factory import metadata as md_factory
from tests.testutils.factory.common import DEFAULT_JIRA_ACCOUNT_ID, DEFAULT_MD_ACCOUNT_ID
from tests.testutils.factory.wizards import insert_repo, pr_models
from tests.testutils.time import dt


Expand Down Expand Up @@ -487,7 +488,8 @@ def validate_pull_request_facts(prmeta: Dict[str, Any], prt: PullRequestFacts):
assert prt.first_review_request <= prt.first_comment_on_first_review
else:
assert not prt.last_review
assert not prt.last_commit_before_first_review
if prt.reviews:
assert not prt.last_commit_before_first_review
if prt.first_review_request_exact:
assert prt.first_review_request
if prt.approved:
Expand Down Expand Up @@ -726,6 +728,66 @@ async def test_pr_facts_miner_empty_releases(
validate_pull_request_facts(*prt)


@with_defer
async def test_pr_facts_first_comment_first_review_not_reviewd(mdb_rw, pdb, rdb, sdb, pr_miner):
async with DBCleaner(mdb_rw) as mdb_cleaner:
repo = md_factory.RepositoryFactory(node_id=99, full_name="o/r")
await insert_repo(repo, mdb_cleaner, mdb_rw, sdb)

models = [
*pr_models(
99,
1,
1,
repository_full_name="o/r",
created_at=dt(2011, 1, 2),
closed_at=dt(2011, 1, 15),
merged_at=dt(2011, 1, 15),
user_login="user0",
merged_by_login="user0",
),
]
mdb_cleaner.add_models(*models)
await models_insert(mdb_rw, *models)

prefixer = await Prefixer.load((DEFAULT_MD_ACCOUNT_ID,), mdb_rw, None)
settings = Settings.from_account(1, prefixer, sdb, mdb_rw, None, None)
release_settings = await settings.list_release_matches()
branches, default_branches = await BranchMiner.load_branches(
{"o/r"}, prefixer, 1, (DEFAULT_MD_ACCOUNT_ID,), mdb_rw, None, None,
)

miner, _, _, _ = await pr_miner.mine(
date(2011, 1, 1),
date(2012, 1, 1),
dt(2011, 1, 1),
dt(2012, 1, 1),
{"o/r"},
{},
LabelFilter.empty(),
JIRAFilter.empty(),
True,
branches,
default_branches,
False,
release_settings,
LogicalRepositorySettings.empty(),
prefixer,
1,
(DEFAULT_MD_ACCOUNT_ID,),
mdb_rw,
pdb,
rdb,
None,
)
facts_miner = PullRequestFactsMiner({})
prs_facts = [facts_miner(mined_pr) for mined_pr in miner]
pr_facts = prs_facts[0]
assert pr_facts.first_review_request == np.datetime64(datetime(2011, 1, 15))
assert pr_facts.first_review_request_exact is None
assert pr_facts.first_comment_on_first_review is None


@with_defer
async def test_pr_mine_by_ids(
branches,
Expand Down

0 comments on commit 506d785

Please sign in to comment.