Merge agent-A: move feedback*.py from WIP into tree
This commit is contained in:
Marcos
@@ -0,0 +1,317 @@
|
||||
"""Tests for pilot/feedback.py — SQLite storage for review feedback signals.
|
||||
|
||||
Covers: schema bootstrap, posthash stability, dedup-on-insert, reaction /
|
||||
thread-state / reply upserts, the analyzer-side `findings_with_votes` join,
|
||||
and graceful failure on bad inputs.
|
||||
"""
|
||||
import sqlite3
|
||||
import tempfile
|
||||
import unittest
|
||||
|
||||
from pilot import feedback
|
||||
|
||||
|
||||
class TestPosthash(unittest.TestCase):
|
||||
def test_stable_across_calls(self):
|
||||
a = feedback.posthash("src/api/foo.ts", 42, "HIGH", "Race condition in handler")
|
||||
b = feedback.posthash("src/api/foo.ts", 42, "HIGH", "Race condition in handler")
|
||||
self.assertEqual(a, b)
|
||||
|
||||
def test_length_is_short(self):
|
||||
h = feedback.posthash("a", 1, "low", "x")
|
||||
self.assertEqual(len(h), 16)
|
||||
|
||||
def test_different_path_different_hash(self):
|
||||
self.assertNotEqual(
|
||||
feedback.posthash("a", 1, "LOW", "x"),
|
||||
feedback.posthash("b", 1, "LOW", "x"),
|
||||
)
|
||||
|
||||
def test_different_line_different_hash(self):
|
||||
self.assertNotEqual(
|
||||
feedback.posthash("a", 1, "LOW", "x"),
|
||||
feedback.posthash("a", 2, "LOW", "x"),
|
||||
)
|
||||
|
||||
def test_different_severity_different_hash(self):
|
||||
# Same line, same problem, different severity → different signal.
|
||||
self.assertNotEqual(
|
||||
feedback.posthash("a", 1, "LOW", "x"),
|
||||
feedback.posthash("a", 1, "CRITICAL", "x"),
|
||||
)
|
||||
|
||||
def test_problem_prefix_used_only(self):
|
||||
# First 80 chars participate; rest is ignored.
|
||||
self.assertEqual(
|
||||
feedback.posthash("a", 1, "LOW", "x" * 80 + "tail"),
|
||||
feedback.posthash("a", 1, "LOW", "x" * 80),
|
||||
)
|
||||
|
||||
def test_case_and_whitespace_normalized_in_problem(self):
|
||||
# Lowercased + stripped → same hash.
|
||||
self.assertEqual(
|
||||
feedback.posthash("a", 1, "LOW", " Same Finding "),
|
||||
feedback.posthash("a", 1, "LOW", "same finding"),
|
||||
)
|
||||
|
||||
|
||||
class TestInit(unittest.TestCase):
|
||||
def test_init_creates_db(self):
|
||||
with tempfile.TemporaryDirectory() as d:
|
||||
db = f"{d}/f.db"
|
||||
conn = feedback.init(db)
|
||||
# Application tables exist (sqlite_sequence is a bookkeeping table
|
||||
# created by AUTOINCREMENT — not part of the contract).
|
||||
tables = {r[0] for r in conn.execute(
|
||||
"SELECT name FROM sqlite_master WHERE type='table'"
|
||||
).fetchall()}
|
||||
self.assertTrue(
|
||||
{"review", "inline_finding", "reaction", "thread_state", "reply"}.issubset(tables),
|
||||
f"missing tables: got {tables}",
|
||||
)
|
||||
conn.close()
|
||||
|
||||
def test_init_is_idempotent(self):
|
||||
with tempfile.TemporaryDirectory() as d:
|
||||
db = f"{d}/f.db"
|
||||
feedback.init(db)
|
||||
# Second call must not raise.
|
||||
feedback.init(db)
|
||||
|
||||
|
||||
class TestRecordReview(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.conn = feedback.init(f"{self.tmp.name}/f.db")
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close(); self.tmp.cleanup()
|
||||
|
||||
def test_returns_id_and_row(self):
|
||||
rid = feedback.record_review(
|
||||
self.conn, repo="o/r", pr=1, head_sha="abc",
|
||||
review_id_gitea=99, body_comment_id=42,
|
||||
)
|
||||
self.assertIsNotNone(rid)
|
||||
row = self.conn.execute("SELECT * FROM review WHERE id = ?", (rid,)).fetchone()
|
||||
self.assertEqual(row[1], "o/r")
|
||||
self.assertEqual(row[4], 99)
|
||||
self.assertEqual(row[5], 42)
|
||||
|
||||
|
||||
class TestRecordInlineFinding(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.conn = feedback.init(f"{self.tmp.name}/f.db")
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close(); self.tmp.cleanup()
|
||||
|
||||
def _new_review(self):
|
||||
return feedback.record_review(
|
||||
self.conn, repo="o/r", pr=1, head_sha="x",
|
||||
)
|
||||
|
||||
def test_insert_returns_id(self):
|
||||
rid = self._new_review()
|
||||
fid = feedback.record_inline_finding(
|
||||
self.conn, review_id=rid, repo="o/r", pr=1,
|
||||
path="a/b.ts", line=10, severity="HIGH",
|
||||
problem="bug", fix="patch", suggestion="code",
|
||||
comment_id=555,
|
||||
)
|
||||
self.assertIsNotNone(fid)
|
||||
|
||||
def test_dedup_on_posthash(self):
|
||||
# Two reviews of the SAME finding on different PRs insert two
|
||||
# rows — deduplication by posthash is the *analyzer's* job
|
||||
# (findings_with_votes GROUP BY posthash). Storing one row per
|
||||
# review preserves per-comment reactions across PRs.
|
||||
rid1 = self._new_review()
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=rid1, repo="o/r", pr=1,
|
||||
path="a/b.ts", line=10, severity="HIGH", problem="race",
|
||||
comment_id=100,
|
||||
)
|
||||
rid2 = feedback.record_review(self.conn, repo="o/r", pr=2, head_sha="y")
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=rid2, repo="o/r", pr=2,
|
||||
path="a/b.ts", line=10, severity="HIGH", problem="race",
|
||||
comment_id=200,
|
||||
)
|
||||
rows = self.conn.execute(
|
||||
"SELECT id, comment_id FROM inline_finding WHERE path='a/b.ts' AND line=10 ORDER BY id"
|
||||
).fetchall()
|
||||
self.assertEqual(len(rows), 2)
|
||||
# Both comment_ids preserved (PK dedup is the *reaction* table's job).
|
||||
self.assertEqual([r[1] for r in rows], [100, 200])
|
||||
|
||||
def test_posthash_set(self):
|
||||
rid = self._new_review()
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=rid, repo="o/r", pr=1,
|
||||
path="a", line=1, severity="LOW", problem="nit",
|
||||
)
|
||||
ph = self.conn.execute(
|
||||
"SELECT posthash FROM inline_finding LIMIT 1"
|
||||
).fetchone()[0]
|
||||
expected = feedback.posthash("a", 1, "LOW", "nit")
|
||||
self.assertEqual(ph, expected)
|
||||
|
||||
|
||||
class TestRecordReaction(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.conn = feedback.init(f"{self.tmp.name}/f.db")
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close(); self.tmp.cleanup()
|
||||
|
||||
def test_insert_upsert(self):
|
||||
ok = feedback.record_reaction(
|
||||
self.conn, comment_id=10, user="alice", content="+1",
|
||||
)
|
||||
self.assertTrue(ok)
|
||||
n = self.conn.execute("SELECT COUNT(*) FROM reaction").fetchone()[0]
|
||||
self.assertEqual(n, 1)
|
||||
# Re-insert same PK → no duplicate.
|
||||
feedback.record_reaction(self.conn, comment_id=10, user="alice", content="+1")
|
||||
n = self.conn.execute("SELECT COUNT(*) FROM reaction").fetchone()[0]
|
||||
self.assertEqual(n, 1)
|
||||
|
||||
def test_distinct_users_can_react(self):
|
||||
feedback.record_reaction(self.conn, comment_id=10, user="a", content="+1")
|
||||
feedback.record_reaction(self.conn, comment_id=10, user="b", content="-1")
|
||||
n = self.conn.execute("SELECT COUNT(*) FROM reaction").fetchone()[0]
|
||||
self.assertEqual(n, 2)
|
||||
|
||||
|
||||
class TestRecordThreadState(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.conn = feedback.init(f"{self.tmp.name}/f.db")
|
||||
rid = feedback.record_review(self.conn, repo="o/r", pr=1, head_sha="x")
|
||||
self.fid = feedback.record_inline_finding(
|
||||
self.conn, review_id=rid, repo="o/r", pr=1,
|
||||
path="a", line=1, severity="LOW", problem="x",
|
||||
)
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close(); self.tmp.cleanup()
|
||||
|
||||
def test_upsert_overwrites(self):
|
||||
feedback.record_thread_state(self.conn, finding_id=self.fid, resolved=True)
|
||||
row = self.conn.execute(
|
||||
"SELECT resolved FROM thread_state WHERE finding_id = ?", (self.fid,)
|
||||
).fetchone()
|
||||
self.assertEqual(row[0], 1)
|
||||
feedback.record_thread_state(self.conn, finding_id=self.fid, resolved=False)
|
||||
row = self.conn.execute(
|
||||
"SELECT resolved FROM thread_state WHERE finding_id = ?", (self.fid,)
|
||||
).fetchone()
|
||||
self.assertEqual(row[0], 0)
|
||||
|
||||
|
||||
class TestRecordReply(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.conn = feedback.init(f"{self.tmp.name}/f.db")
|
||||
rid = feedback.record_review(self.conn, repo="o/r", pr=1, head_sha="x")
|
||||
self.fid = feedback.record_inline_finding(
|
||||
self.conn, review_id=rid, repo="o/r", pr=1,
|
||||
path="a", line=1, severity="LOW", problem="x",
|
||||
)
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close(); self.tmp.cleanup()
|
||||
|
||||
def test_insert_idempotent(self):
|
||||
feedback.record_reply(
|
||||
self.conn, finding_id=self.fid, author="a",
|
||||
body="hi", created_at=1000,
|
||||
)
|
||||
feedback.record_reply(
|
||||
self.conn, finding_id=self.fid, author="a",
|
||||
body="hi", created_at=1000, # same PK
|
||||
)
|
||||
n = self.conn.execute("SELECT COUNT(*) FROM reply").fetchone()[0]
|
||||
self.assertEqual(n, 1)
|
||||
|
||||
|
||||
class TestFindingsWithVotes(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.conn = feedback.init(f"{self.tmp.name}/f.db")
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close(); self.tmp.cleanup()
|
||||
|
||||
def _seed(self):
|
||||
rid = feedback.record_review(self.conn, repo="o/r", pr=1, head_sha="x")
|
||||
fid = feedback.record_inline_finding(
|
||||
self.conn, review_id=rid, repo="o/r", pr=1,
|
||||
path="a/b.ts", line=10, severity="HIGH",
|
||||
problem="race", comment_id=500,
|
||||
)
|
||||
feedback.record_reaction(self.conn, comment_id=500, user="u1", content="+1")
|
||||
feedback.record_reaction(self.conn, comment_id=500, user="u2", content="-1")
|
||||
feedback.record_thread_state(self.conn, finding_id=fid, resolved=True)
|
||||
feedback.record_reply(
|
||||
self.conn, finding_id=fid, author="u3",
|
||||
body="this is fine because of X", created_at=2000,
|
||||
)
|
||||
return fid
|
||||
|
||||
def test_join_rolls_up_votes(self):
|
||||
self._seed()
|
||||
rows = list(feedback.findings_with_votes(self.conn))
|
||||
self.assertEqual(len(rows), 1)
|
||||
r = rows[0]
|
||||
self.assertEqual(r["upvotes"], 1)
|
||||
self.assertEqual(r["downvotes"], 1)
|
||||
self.assertEqual(r["resolved"], 1)
|
||||
self.assertEqual(r["reply_count"], 1)
|
||||
self.assertIn("this is fine", r["reply_bodies"])
|
||||
|
||||
def test_repo_filter(self):
|
||||
self._seed()
|
||||
# Add a finding under a different repo.
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=None, repo="other/r", pr=99,
|
||||
path="x", line=1, severity="LOW", problem="y",
|
||||
)
|
||||
rows = list(feedback.findings_with_votes(self.conn, repo="o/r"))
|
||||
self.assertEqual(len(rows), 1)
|
||||
self.assertEqual(rows[0]["repo"], "o/r")
|
||||
|
||||
def test_findings_with_no_signals_return_zero_votes(self):
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=None, repo="x/y", pr=1,
|
||||
path="p", line=1, severity="LOW", problem="z",
|
||||
)
|
||||
rows = list(feedback.findings_with_votes(self.conn))
|
||||
self.assertEqual(len(rows), 1)
|
||||
self.assertEqual(rows[0]["upvotes"], 0)
|
||||
self.assertEqual(rows[0]["downvotes"], 0)
|
||||
self.assertIsNone(rows[0]["resolved"])
|
||||
|
||||
|
||||
class TestKnownPosthashes(unittest.TestCase):
|
||||
def test_returns_distinct_set(self):
|
||||
with tempfile.TemporaryDirectory() as d:
|
||||
conn = feedback.init(f"{d}/f.db")
|
||||
feedback.record_inline_finding(
|
||||
conn, review_id=None, repo="o/r", pr=1,
|
||||
path="a", line=1, severity="LOW", problem="x",
|
||||
)
|
||||
feedback.record_inline_finding(
|
||||
conn, review_id=None, repo="o/r", pr=1,
|
||||
path="a", line=2, severity="LOW", problem="y",
|
||||
)
|
||||
phs = feedback.known_posthashes_for_repo(conn, "o/r")
|
||||
self.assertEqual(len(phs), 2)
|
||||
conn.close()
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -0,0 +1,231 @@
|
||||
"""Tests for pilot/feedback_analyze.py.
|
||||
|
||||
Verify:
|
||||
- empty DB produces a friendly empty-state report (no crash)
|
||||
- findings are aggregated by posthash across multiple PRs
|
||||
- net false-positive score weights downvotes + unresolved + negation
|
||||
replies; acceptance weights upvotes + resolved
|
||||
- restraint metric reports the right ratio
|
||||
- case-review queue lists every disagreement
|
||||
- markdown + JSON output modes both work
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import unittest
|
||||
|
||||
HERE = os.path.dirname(os.path.abspath(__file__))
|
||||
sys.path.insert(0, os.path.join(HERE, "..", "..", "pilot"))
|
||||
|
||||
import feedback # noqa: E402
|
||||
import feedback_analyze # noqa: E402
|
||||
|
||||
|
||||
def _seed(conn, findings):
|
||||
"""Helper: insert a list of (repo, pr, path, line, severity, problem,
|
||||
[reaction users/contents], [reply bodies], [resolved]) tuples.
|
||||
Each finding gets a fresh review row + a unique comment_id so the
|
||||
reaction-join in `findings_with_votes` matches."""
|
||||
for f in findings:
|
||||
(repo, pr_idx, path, line, sev, problem, reacts, replies,
|
||||
resolved) = f
|
||||
rid = feedback.record_review(conn, repo=repo, pr=pr_idx, head_sha="x")
|
||||
cid = (hash((repo, pr_idx, path, line, sev, problem)) & 0xFFFFFFFF) or 1
|
||||
fid = feedback.record_inline_finding(
|
||||
conn, review_id=rid, repo=repo, pr=pr_idx,
|
||||
path=path, line=line, severity=sev, problem=problem,
|
||||
comment_id=cid,
|
||||
)
|
||||
for user, content in reacts:
|
||||
feedback.record_reaction(
|
||||
conn, comment_id=cid, user=user, content=content,
|
||||
)
|
||||
for i, body in enumerate(replies):
|
||||
feedback.record_reply(
|
||||
conn, finding_id=fid, author="alice",
|
||||
body=body, created_at=1000 + i,
|
||||
)
|
||||
if resolved is not None:
|
||||
feedback.record_thread_state(
|
||||
conn, finding_id=fid, resolved=resolved,
|
||||
)
|
||||
|
||||
|
||||
class TestEmptyState(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
|
||||
def tearDown(self):
|
||||
self.tmp.cleanup()
|
||||
|
||||
def test_empty_db_markdown_does_not_crash(self):
|
||||
report = feedback_analyze.analyze(self.db)
|
||||
self.assertIn("# pragent feedback report", report)
|
||||
self.assertIn("findings analyzed**: 0", report)
|
||||
self.assertIn("Restraint", report)
|
||||
|
||||
def test_empty_db_json_has_zero_findings(self):
|
||||
report = feedback_analyze.analyze(self.db, as_json=True)
|
||||
d = json.loads(report)
|
||||
self.assertEqual(d["total_findings"], 0)
|
||||
self.assertEqual(d["restraint"]["total"], 0)
|
||||
|
||||
|
||||
class TestScoring(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
self.conn = feedback.init(self.db)
|
||||
# Two PRs, three findings:
|
||||
# A: 👍×2, resolved=true → acceptance
|
||||
# B: 👎×2, unresolved, "false positive" reply → false-positive
|
||||
# C: no signals → ignored
|
||||
_seed(self.conn, [
|
||||
("o/r", 1, "a.ts", 10, "HIGH", "race in handler",
|
||||
[("u1", "+1"), ("u2", "+1")], [], True),
|
||||
("o/r", 1, "b.ts", 20, "LOW", "missing semicolon",
|
||||
[("u1", "-1"), ("u2", "-1")],
|
||||
["False positive — this is fine."], False),
|
||||
("o/r", 1, "c.ts", 30, "INFO", "naming nit",
|
||||
[], [], None),
|
||||
])
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close()
|
||||
self.tmp.cleanup()
|
||||
|
||||
def test_accepted_ranked_above_fp(self):
|
||||
d = json.loads(feedback_analyze.analyze(self.db, as_json=True))
|
||||
self.assertEqual(len(d["top_accepted"]), 1)
|
||||
self.assertEqual(d["top_accepted"][0]["path"], "a.ts")
|
||||
self.assertEqual(len(d["top_false_positive"]), 1)
|
||||
self.assertEqual(d["top_false_positive"][0]["path"], "b.ts")
|
||||
|
||||
def test_fp_score_combines_signals(self):
|
||||
d = json.loads(feedback_analyze.analyze(self.db, as_json=True))
|
||||
fp = d["top_false_positive"][0]
|
||||
# 2 downvotes + 1 unresolved + 2 (negation phrase) = 5
|
||||
self.assertEqual(fp["fp_score"], 5)
|
||||
|
||||
def test_acceptance_score(self):
|
||||
d = json.loads(feedback_analyze.analyze(self.db, as_json=True))
|
||||
ac = d["top_accepted"][0]
|
||||
# 2 upvotes + 1 resolved = 3
|
||||
self.assertEqual(ac["ac_score"], 3)
|
||||
|
||||
def test_case_queue_contains_only_disagreements(self):
|
||||
d = json.loads(feedback_analyze.analyze(self.db, as_json=True))
|
||||
queue = d["case_review_queue"]
|
||||
self.assertEqual(len(queue), 1)
|
||||
self.assertEqual(queue[0]["path"], "b.ts")
|
||||
|
||||
def test_no_signal_finding_is_ignored(self):
|
||||
d = json.loads(feedback_analyze.analyze(self.db, as_json=True))
|
||||
# c.ts has no votes, no replies → not in either top list.
|
||||
paths = {e["path"] for e in d["top_accepted"]}
|
||||
paths.update(e["path"] for e in d["top_false_positive"])
|
||||
self.assertNotIn("c.ts", paths)
|
||||
|
||||
|
||||
class TestRestraint(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
self.conn = feedback.init(self.db)
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close()
|
||||
self.tmp.cleanup()
|
||||
|
||||
def test_high_ratio_triggers_recommendation(self):
|
||||
# 3 reviews, all with findings → 100% "noisy".
|
||||
for pr_i in range(3):
|
||||
feedback.record_review(self.conn, repo="o/r", pr=pr_i, head_sha="x")
|
||||
# Distinct (path, line) per PR so posthash doesn't dedup.
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=None, repo="o/r", pr=pr_i,
|
||||
path=f"a{pr_i}.ts", line=1, severity="LOW",
|
||||
problem=f"x {pr_i}",
|
||||
)
|
||||
report = feedback_analyze.analyze(self.db)
|
||||
self.assertIn("⚠️", report)
|
||||
self.assertIn("100%", report)
|
||||
|
||||
def test_low_ratio_passes(self):
|
||||
# 4 reviews, 1 with findings → 25% noisy = at threshold.
|
||||
for pr_i in range(4):
|
||||
feedback.record_review(self.conn, repo="o/r", pr=pr_i, head_sha="x")
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=None, repo="o/r", pr=0,
|
||||
path="a.ts", line=1, severity="LOW", problem="x",
|
||||
)
|
||||
report = feedback_analyze.analyze(self.db)
|
||||
self.assertIn("✅", report)
|
||||
|
||||
|
||||
class TestMarkdownOutput(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
self.conn = feedback.init(self.db)
|
||||
_seed(self.conn, [
|
||||
("o/r", 1, "a.ts", 10, "HIGH", "race in handler",
|
||||
[("u1", "+1")], [], True),
|
||||
])
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close()
|
||||
self.tmp.cleanup()
|
||||
|
||||
def test_report_has_sections(self):
|
||||
r = feedback_analyze.analyze(self.db)
|
||||
for section in (
|
||||
"# pragent feedback report",
|
||||
"## Restraint",
|
||||
"## Top",
|
||||
"## Case-review queue",
|
||||
"## Where this report goes",
|
||||
):
|
||||
self.assertIn(section, r)
|
||||
|
||||
def test_doordash_rule_quoted(self):
|
||||
r = feedback_analyze.analyze(self.db)
|
||||
# The "noise on clean code" sentence from the DoorDash recap.
|
||||
self.assertIn("noise on clean code", r)
|
||||
|
||||
|
||||
class TestPosthashAggregation(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
self.conn = feedback.init(self.db)
|
||||
# Same finding on three PRs → one aggregated row.
|
||||
# Each PR has its own review + finding (comment_id differs but
|
||||
# posthash is identical, so they collapse on aggregation).
|
||||
for pr_i in range(3):
|
||||
rid = feedback.record_review(self.conn, repo="o/r", pr=pr_i, head_sha="x")
|
||||
feedback.record_inline_finding(
|
||||
self.conn, review_id=rid, repo="o/r", pr=pr_i,
|
||||
path="a.ts", line=10, severity="HIGH",
|
||||
problem="identical problem text",
|
||||
comment_id=1000 + pr_i,
|
||||
)
|
||||
feedback.record_reaction(
|
||||
self.conn, comment_id=1000 + pr_i, user="u", content="+1",
|
||||
)
|
||||
|
||||
def tearDown(self):
|
||||
self.conn.close()
|
||||
self.tmp.cleanup()
|
||||
|
||||
def test_three_occurrences_one_row(self):
|
||||
d = json.loads(feedback_analyze.analyze(self.db, as_json=True))
|
||||
self.assertEqual(len(d["top_accepted"]), 1)
|
||||
self.assertEqual(d["top_accepted"][0]["occurrences"], 3)
|
||||
self.assertEqual(d["top_accepted"][0]["ac_score"], 3)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -0,0 +1,243 @@
|
||||
"""Tests for pilot/feedback_harvest.py.
|
||||
|
||||
Mock `gitea_get` so we exercise the harvester's flow against canned Gitea
|
||||
responses. Verify:
|
||||
- bot-authored reviews only are processed
|
||||
- reactions + thread state + replies all get recorded
|
||||
- best-effort failures don't raise (one bad endpoint shouldn't kill the
|
||||
whole harvest)
|
||||
- posthash dedup: harvesting the same PR twice does NOT double-count
|
||||
reactions.
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
|
||||
HERE = os.path.dirname(os.path.abspath(__file__))
|
||||
sys.path.insert(0, os.path.join(HERE, "..", "..", "pilot"))
|
||||
|
||||
import ai_review # noqa: E402
|
||||
import feedback # noqa: E402
|
||||
import feedback_harvest # noqa: E402
|
||||
|
||||
|
||||
def _make_fake_gitea(routes: dict):
|
||||
"""Build a stand-in for `ai_review.gitea_get` that returns canned bodies.
|
||||
|
||||
`routes` maps relative path (substring) → (status, json_body). Sort
|
||||
keys longest-first so e.g. `pulls/5/reviews/100/comments` matches
|
||||
before `pulls/5/reviews` (which is also a substring of the longer
|
||||
path).
|
||||
"""
|
||||
def fake(api, repo, path, token, accept="application/json"):
|
||||
for needle in sorted(routes.keys(), key=len, reverse=True):
|
||||
if needle in path:
|
||||
status, body = routes[needle]
|
||||
return status, json.dumps(body).encode()
|
||||
return 404, b'{"message":"not found"}'
|
||||
return fake
|
||||
|
||||
|
||||
def _patch(fake):
|
||||
"""Apply the fake to ai_review.gitea_get and feedback_harvest's import."""
|
||||
return patch("ai_review.gitea_get", side_effect=fake)
|
||||
|
||||
|
||||
class TestHarvestForPr(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
|
||||
def tearDown(self):
|
||||
self.tmp.cleanup()
|
||||
|
||||
def _review_payload(self, body="", commit_id="abc123", review_id=100):
|
||||
return [{
|
||||
"id": review_id, "user": {"login": "pragent-bot"},
|
||||
"commit_id": commit_id, "body": body,
|
||||
"created_at": "2026-08-20T10:00:00Z",
|
||||
}]
|
||||
|
||||
def _inline_payload(self, comment_id=500, body="**[LOW]** x", path="a/b.ts", position=42, resolver=""):
|
||||
return [{
|
||||
"id": comment_id, "path": path, "position": position,
|
||||
"body": body, "resolver": resolver,
|
||||
}]
|
||||
|
||||
def test_happy_path_records_reaction_and_thread(self):
|
||||
fake = _make_fake_gitea({
|
||||
"pulls/5/reviews": (200, self._review_payload()),
|
||||
"pulls/5/reviews/100/comments": (200, self._inline_payload(resolver="masi")),
|
||||
"issues/comments/500/reactions": (200, [
|
||||
{"user": {"login": "alice"}, "content": "+1",
|
||||
"created_at": "2026-08-20T11:00:00Z"},
|
||||
{"user": {"login": "bob"}, "content": "-1",
|
||||
"created_at": "2026-08-20T11:01:00Z"},
|
||||
]),
|
||||
"issues/5/comments": (200, []), # no replies
|
||||
})
|
||||
with _patch(fake):
|
||||
stats = feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
self.assertEqual(stats["reviews_seen"], 1)
|
||||
self.assertEqual(stats["findings_seen"], 1)
|
||||
self.assertEqual(stats["reactions_recorded"], 2)
|
||||
self.assertEqual(stats["thread_states_recorded"], 1)
|
||||
# DB should have 1 review, 1 finding, 2 reactions, 1 thread_state
|
||||
conn = feedback.init(self.db)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM review").fetchone()[0], 1,
|
||||
)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM inline_finding").fetchone()[0], 1,
|
||||
)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM reaction").fetchone()[0], 2,
|
||||
)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT resolved FROM thread_state").fetchone()[0], 1,
|
||||
)
|
||||
conn.close()
|
||||
|
||||
def test_skips_non_bot_reviews(self):
|
||||
fake = _make_fake_gitea({
|
||||
"pulls/5/reviews": (200, [{
|
||||
"id": 999, "user": {"login": "masi"}, # not the bot
|
||||
"commit_id": "x", "body": "", "created_at": "2026-08-20T10:00:00Z",
|
||||
}]),
|
||||
})
|
||||
with _patch(fake):
|
||||
stats = feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
self.assertEqual(stats["reviews_seen"], 0)
|
||||
conn = feedback.init(self.db)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM review").fetchone()[0], 0,
|
||||
)
|
||||
conn.close()
|
||||
|
||||
def test_review_list_failure_does_not_raise(self):
|
||||
fake = _make_fake_gitea({
|
||||
"pulls/5/reviews": (500, None),
|
||||
})
|
||||
with _patch(fake):
|
||||
stats = feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
self.assertEqual(stats["reviews_seen"], 0)
|
||||
self.assertGreaterEqual(stats["errors"], 1)
|
||||
|
||||
def test_reactions_endpoint_returns_null_is_tolerated(self):
|
||||
# Some Gitea endpoints return JSON `null` for empty lists. We must
|
||||
# not crash — treat it as "no reactions".
|
||||
fake = _make_fake_gitea({
|
||||
"pulls/5/reviews": (200, self._review_payload()),
|
||||
"pulls/5/reviews/100/comments": (200, self._inline_payload()),
|
||||
"issues/comments/500/reactions": (200, None),
|
||||
"issues/5/comments": (200, []),
|
||||
})
|
||||
with _patch(fake):
|
||||
stats = feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
self.assertEqual(stats["reactions_recorded"], 0)
|
||||
|
||||
def test_reactions_dedup_via_pk_across_harvests(self):
|
||||
# Two harvests of the same PR — both produce an inline_finding row,
|
||||
# but reactions are PK-deduped on (comment_id, user, content) so
|
||||
# the SECOND harvest does NOT double-record them.
|
||||
fake = _make_fake_gitea({
|
||||
"pulls/5/reviews": (200, self._review_payload()),
|
||||
"pulls/5/reviews/100/comments": (200, self._inline_payload()),
|
||||
"issues/comments/500/reactions": (200, [
|
||||
{"user": {"login": "alice"}, "content": "+1",
|
||||
"created_at": "2026-08-20T11:00:00Z"},
|
||||
]),
|
||||
"issues/5/comments": (200, []),
|
||||
})
|
||||
with _patch(fake):
|
||||
feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
conn = feedback.init(self.db)
|
||||
# Two findings (no DB-level posthash UNIQUE), one reaction.
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM inline_finding").fetchone()[0], 2,
|
||||
)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM reaction").fetchone()[0], 1,
|
||||
)
|
||||
conn.close()
|
||||
|
||||
def test_replies_with_review_comment_id_recorded(self):
|
||||
fake = _make_fake_gitea({
|
||||
"pulls/5/reviews": (200, self._review_payload()),
|
||||
"pulls/5/reviews/100/comments": (200, self._inline_payload(comment_id=500)),
|
||||
"issues/comments/500/reactions": (200, []),
|
||||
"issues/5/comments": (200, [{
|
||||
"id": 900, "review_comment_id": 500,
|
||||
"user": {"login": "alice"},
|
||||
"body": "False positive — this is intentional",
|
||||
"created_at": "2026-08-20T12:00:00Z",
|
||||
}]),
|
||||
})
|
||||
with _patch(fake):
|
||||
stats = feedback_harvest.harvest_for_pr(
|
||||
api="http://x", token="t", repo="o/r", pr_index=5,
|
||||
db_path=self.db,
|
||||
)
|
||||
self.assertEqual(stats["replies_recorded"], 1)
|
||||
conn = feedback.init(self.db)
|
||||
self.assertEqual(
|
||||
conn.execute("SELECT COUNT(*) FROM reply").fetchone()[0], 1,
|
||||
)
|
||||
conn.close()
|
||||
|
||||
|
||||
class TestParseHelpers(unittest.TestCase):
|
||||
def test_severity_extracted(self):
|
||||
self.assertEqual(
|
||||
feedback_harvest._parse_severity("**[HIGH]** race in foo"),
|
||||
"HIGH",
|
||||
)
|
||||
|
||||
def test_severity_defaults_to_info(self):
|
||||
self.assertEqual(feedback_harvest._parse_severity("plain text"), "INFO")
|
||||
|
||||
def test_path_line_extracted(self):
|
||||
p, l = feedback_harvest._parse_path_line("see `src/foo.ts:42` here")
|
||||
self.assertEqual(p, "src/foo.ts")
|
||||
self.assertEqual(l, 42)
|
||||
|
||||
def test_negation_phrases_caught(self):
|
||||
self.assertTrue(feedback_harvest._is_negation_reply("This is intentional."))
|
||||
self.assertTrue(feedback_harvest._is_negation_reply("false positive — see X"))
|
||||
self.assertFalse(feedback_harvest._is_negation_reply("thanks for catching this!"))
|
||||
# Empty / None safe
|
||||
self.assertFalse(feedback_harvest._is_negation_reply(""))
|
||||
self.assertFalse(feedback_harvest._is_negation_reply(None))
|
||||
|
||||
def test_classify_reaction(self):
|
||||
self.assertEqual(feedback_harvest.classify_reaction("+1"), "positive")
|
||||
self.assertEqual(feedback_harvest.classify_reaction("-1"), "negative")
|
||||
self.assertEqual(feedback_harvest.classify_reaction("rocket"), "positive")
|
||||
self.assertEqual(feedback_harvest.classify_reaction("confused"), "negative")
|
||||
self.assertEqual(feedback_harvest.classify_reaction("eyes"), "neutral")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -0,0 +1,97 @@
|
||||
"""Tests for pilot/feedback_post.py — report delivery to Gitea.
|
||||
|
||||
Mock `ai_review.gitea_get` + `gitea_post` so we exercise the find-or-create
|
||||
+ comment-post flow without hitting the real API.
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
|
||||
HERE = os.path.dirname(os.path.abspath(__file__))
|
||||
sys.path.insert(0, os.path.join(HERE, "..", "..", "pilot"))
|
||||
|
||||
import feedback # noqa: E402
|
||||
import feedback_analyze # noqa: E402
|
||||
import feedback_post # noqa: E402
|
||||
|
||||
|
||||
def _make_fake(method_routes: dict):
|
||||
"""`method_routes` maps HTTP path substring → (status, body, method).
|
||||
|
||||
For our purposes both gitea_get and gitea_post share the same fake —
|
||||
gitea_get is GET, gitea_post is POST, and the post helper also has a
|
||||
body param. The fake returns whatever the route's body says.
|
||||
"""
|
||||
def fake_get(api, repo, path, token, accept="application/json"):
|
||||
for needle in sorted(method_routes.keys(), key=len, reverse=True):
|
||||
status, body, _m = method_routes[needle]
|
||||
if needle in path:
|
||||
return status, json.dumps(body).encode()
|
||||
return 404, b'{"message":"not found"}'
|
||||
|
||||
def fake_post(api, repo, path, token, body):
|
||||
for needle in sorted(method_routes.keys(), key=len, reverse=True):
|
||||
status, resp_body, _m = method_routes[needle]
|
||||
if needle in path:
|
||||
return status, json.dumps(resp_body).encode()
|
||||
return 404, b'{"message":"not found"}'
|
||||
|
||||
return fake_get, fake_post
|
||||
|
||||
|
||||
class TestDeliver(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self.tmp = tempfile.TemporaryDirectory()
|
||||
self.db = f"{self.tmp.name}/f.db"
|
||||
conn = feedback.init(self.db)
|
||||
rid = feedback.record_review(conn, repo="o/r", pr=1, head_sha="x")
|
||||
feedback.record_inline_finding(
|
||||
conn, review_id=rid, repo="o/r", pr=1,
|
||||
path="a.ts", line=1, severity="HIGH",
|
||||
problem="x", comment_id=99,
|
||||
)
|
||||
conn.close()
|
||||
|
||||
def tearDown(self):
|
||||
self.tmp.cleanup()
|
||||
|
||||
def test_creates_issue_then_posts_comment(self):
|
||||
routes = {
|
||||
"issues?state=open": (200, [], "GET"), # no existing issue
|
||||
"issues": (201, {"id": 42, "number": 7, "title": "..."}, "POST"),
|
||||
"issues/7/comments": (201, {"id": 777}, "POST"),
|
||||
}
|
||||
fake_get, fake_post = _make_fake(routes)
|
||||
with patch("ai_review.gitea_get", side_effect=fake_get), \
|
||||
patch("ai_review.gitea_post", side_effect=fake_post):
|
||||
stats = feedback_post.deliver(
|
||||
api="http://x", token="t", db_path=self.db,
|
||||
repo="gitea_admin/pragent", title="pragent feedback roll-up",
|
||||
)
|
||||
self.assertEqual(stats["issue_id"], 7)
|
||||
self.assertEqual(stats["comment_id"], 777)
|
||||
|
||||
def test_reuses_existing_issue(self):
|
||||
routes = {
|
||||
"issues?state=open": (200, [
|
||||
{"id": 99, "number": 9, "title": "pragent feedback roll-up"},
|
||||
{"id": 100, "number": 10, "title": "something else"},
|
||||
], "GET"),
|
||||
"issues/9/comments": (201, {"id": 888}, "POST"),
|
||||
}
|
||||
fake_get, fake_post = _make_fake(routes)
|
||||
with patch("ai_review.gitea_get", side_effect=fake_get), \
|
||||
patch("ai_review.gitea_post", side_effect=fake_post):
|
||||
stats = feedback_post.deliver(
|
||||
api="http://x", token="t", db_path=self.db,
|
||||
repo="gitea_admin/pragent", title="pragent feedback roll-up",
|
||||
)
|
||||
self.assertEqual(stats["issue_id"], 9)
|
||||
self.assertEqual(stats["comment_id"], 888)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user