Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions .github/actions/bot-autoassign/issue_assignment_bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def handle_issue_comment(self):
try:
if self.event_payload.get("issue", {}).get("pull_request"):
print("Comment is on a PR, not an issue - skipping")
return False
return True
comment = self.event_payload.get("comment", {})
issue = self.event_payload.get("issue", {})
comment_body = comment.get("body", "")
Expand All @@ -285,7 +285,7 @@ def handle_issue_comment(self):
if self.is_assignment_request(comment_body):
return self.respond_to_assignment_request(issue_number, commenter)
print("Comment does not contain assignment request")
return False
return True
except Exception as e:
print(f"Error handling issue comment: {e}")
return False
Expand All @@ -308,10 +308,13 @@ def handle_pull_request(self):
# We consider the event handled even if no issues were linked
return True
elif action == "closed":
self.unassign_issues_from_pr(pr_body, pr_author)
if pr.get("merged", False):
print(f"PR #{pr_number} was merged, keeping issue assignments")
else:
self.unassign_issues_from_pr(pr_body, pr_author)
return True
print(f"PR action '{action}' not handled")
return False
return True
except Exception as e:
print(f"Error handling pull request: {e}")
return False
Expand All @@ -328,7 +331,7 @@ def run(self):
return self.handle_pull_request()
else:
print(f"Event type '{self.event_name}'" " not supported")
return False
return True
except Exception as e:
print(f"Error in main execution: {e}")
return False
Expand Down
12 changes: 6 additions & 6 deletions .github/actions/bot-autoassign/pr_reopen_bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def handle_pr_reopen(self):
print(f"Reassigned {len(reassigned)}" f" issues to {pr_author}")
return True
except Exception as e:
print(f"Error handling PR reopen: {e}")
print(f"Error handling reopened PR: {e}")
return False

def run(self):
Expand All @@ -89,7 +89,7 @@ def run(self):
return self.handle_pr_reopen()
else:
print(f"Event type '{self.event_name}'" " not supported")
return False
return True
except Exception as e:
print(f"Error in main execution: {e}")
return False
Expand All @@ -113,15 +113,15 @@ def handle_contributor_activity(self):
return False
if not issue_data.get("pull_request"):
print("Comment is on an issue," " not a PR, skipping")
return False
return True
pr = self.repo.get_pull(pr_number)
if not pr.user or commenter != pr.user.login:
print("Comment not from PR author, skipping")
return False
return True
labels = [label.name for label in pr.get_labels()]
if "stale" not in labels:
print("PR is not stale, skipping")
return False
return True
try:
pr.remove_from_labels("stale")
print("Removed stale label")
Expand Down Expand Up @@ -169,7 +169,7 @@ def run(self):
return self.handle_contributor_activity()
else:
print(f"Event type '{self.event_name}'" " not supported")
return False
return True
except Exception as e:
print(f"Error in main execution: {e}")
return False
Expand Down
2 changes: 1 addition & 1 deletion .github/actions/bot-autoassign/stale_pr_bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def unassign_linked_issues(self, pr):
def close_stale_pr(self, pr, days_inactive):
if pr.state == "closed":
print(f"PR #{pr.number} is already closed, skipping")
return False
return True
try:
pr_author = pr.user.login if pr.user else None
if not pr_author:
Expand Down
24 changes: 20 additions & 4 deletions .github/actions/bot-autoassign/tests/test_issue_assignment_bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ def test_skip_pr_comment(self, bot_env):
},
}
)
assert not bot.handle_issue_comment()
assert bot.handle_issue_comment()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Add no-side-effect assertions for skip paths.

Since these now return handled=True, please also assert they do not perform assignment-response side effects (e.g., no repo.get_issue / no issue comment creation).

Based on learnings: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior.

Also applies to: 307-307

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py at line
294, The test currently only asserts that bot.handle_issue_comment() returns
truthy for skip paths; update it to also assert no side-effect calls were made
(e.g., ensure repo.get_issue() and any issue comment creation methods were not
invoked). Specifically, in the test surrounding assert
bot.handle_issue_comment() add negative assertions on the mocked
repository/issue methods used by the bot (for example, assert_not_called on
repo.get_issue and on the issue comment creation mock), and apply the same
additional no-side-effect assertions to the other similar assertion at the later
test location (the one around line 307) so both skip-paths confirm handled=True
and no repo/issue interactions occurred.


def test_non_assignment_comment(self, bot_env):
bot = IssueAssignmentBot()
Expand All @@ -304,7 +304,7 @@ def test_non_assignment_comment(self, bot_env):
},
}
)
assert not bot.handle_issue_comment()
assert bot.handle_issue_comment()

def test_no_payload(self, bot_env):
bot = IssueAssignmentBot()
Expand Down Expand Up @@ -373,6 +373,22 @@ def test_closed(self, bot_env):
assert bot.handle_pull_request()
mock_issue.remove_from_assignees.assert_called_once_with("testuser")

def test_merged_does_not_unassign(self, bot_env):
bot = IssueAssignmentBot()
bot.load_event_payload(
{
"action": "closed",
"pull_request": {
"number": 100,
"user": {"login": "testuser"},
"body": "Fixes #123",
"merged": True,
},
}
)
assert bot.handle_pull_request()
bot_env["repo"].get_issue.assert_not_called()

def test_unsupported_action(self, bot_env):
bot = IssueAssignmentBot()
bot.load_event_payload(
Expand All @@ -385,7 +401,7 @@ def test_unsupported_action(self, bot_env):
},
}
)
assert not bot.handle_pull_request()
assert bot.handle_pull_request()


class TestRun:
Expand Down Expand Up @@ -431,7 +447,7 @@ def test_pull_request_event(self, bot_env):
def test_unsupported_event(self, bot_env):
bot = IssueAssignmentBot()
bot.event_name = "push"
assert not bot.run()
assert bot.run()

def test_no_github_client(self, bot_env):
bot = IssueAssignmentBot()
Expand Down
10 changes: 5 additions & 5 deletions .github/actions/bot-autoassign/tests/test_pr_reopen_bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def test_handle_pr_reopen_no_payload(self, bot_env):
def test_run_unsupported_event(self, bot_env):
bot = PRReopenBot()
bot.event_name = "push"
assert not bot.run()
assert bot.run()


class TestPRActivityBot:
Expand Down Expand Up @@ -160,7 +160,7 @@ def test_handle_contributor_activity_not_author(self, bot_env):
mock_pr = Mock()
mock_pr.user.login = "testuser"
bot_env["repo"].get_pull.return_value = mock_pr
assert not bot.handle_contributor_activity()
assert bot.handle_contributor_activity()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Strengthen skip-path tests with side-effect assertions.

These tests currently assert truthy return only; please also assert no reassignment/label/comment side effects occurred in each skip branch.

✅ Suggested test hardening
     def test_handle_contributor_activity_not_author(self, bot_env):
         ...
         bot_env["repo"].get_pull.return_value = mock_pr
         assert bot.handle_contributor_activity()
+        mock_pr.remove_from_labels.assert_not_called()
+        mock_pr.create_issue_comment.assert_not_called()

     def test_handle_contributor_activity_pr_not_stale(self, bot_env):
         ...
         bot_env["repo"].get_pull.return_value = mock_pr
         assert bot.handle_contributor_activity()
+        mock_pr.remove_from_labels.assert_not_called()
+        mock_pr.create_issue_comment.assert_not_called()

     def test_handle_contributor_activity_not_pr(self, bot_env):
         ...
         assert bot.handle_contributor_activity()
+        bot_env["repo"].get_pull.assert_not_called()

Also applies to: 180-180, 193-193

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/actions/bot-autoassign/tests/test_pr_reopen_bot.py at line 163, The
tests only assert that bot.handle_contributor_activity() returns truthy for skip
branches but don't verify no side-effects occurred; update each skip-path test
(the ones calling bot.handle_contributor_activity()) to also assert that the
mocked side-effect methods (the repository/github client methods used by the bot
to reassign PRs, add/remove labels, or post comments) were not called—i.e.,
after calling bot.handle_contributor_activity() assert
mock_reassign.assert_not_called(), mock_add_label.assert_not_called(),
mock_remove_label.assert_not_called(), and
mock_create_comment.assert_not_called() (use the actual mock names used in these
tests) so each skip branch verifies no reassignment/label/comment side effects
occurred.


def test_handle_contributor_activity_pr_not_stale(self, bot_env):
bot = PRActivityBot()
Expand All @@ -177,7 +177,7 @@ def test_handle_contributor_activity_pr_not_stale(self, bot_env):
mock_pr.user.login = "testuser"
mock_pr.get_labels.return_value = []
bot_env["repo"].get_pull.return_value = mock_pr
assert not bot.handle_contributor_activity()
assert bot.handle_contributor_activity()

def test_handle_contributor_activity_not_pr(self, bot_env):
bot = PRActivityBot()
Expand All @@ -190,7 +190,7 @@ def test_handle_contributor_activity_not_pr(self, bot_env):
"comment": {"user": {"login": "testuser"}},
}
)
assert not bot.handle_contributor_activity()
assert bot.handle_contributor_activity()

def test_handle_contributor_activity_no_payload(self, bot_env):
bot = PRActivityBot()
Expand All @@ -199,4 +199,4 @@ def test_handle_contributor_activity_no_payload(self, bot_env):
def test_run_unsupported_event(self, bot_env):
bot = PRActivityBot()
bot.event_name = "push"
assert not bot.run()
assert bot.run()
2 changes: 1 addition & 1 deletion .github/actions/bot-autoassign/tests/test_stale_pr_bot.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ def test_already_closed(self, bot_env):
bot = StalePRBot()
mock_pr = Mock()
mock_pr.state = "closed"
assert not bot.close_stale_pr(mock_pr, 60)
assert bot.close_stale_pr(mock_pr, 60)
mock_pr.create_issue_comment.assert_not_called()
mock_pr.edit.assert_not_called()

Expand Down
1 change: 1 addition & 0 deletions .github/workflows/bot-autoassign-pr-issue-link.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ concurrency:

jobs:
auto-assign-issue:
if: github.event.action != 'closed' || github.event.pull_request.merged == false
runs-on: ubuntu-latest
steps:
- name: Generate GitHub App token
Expand Down
53 changes: 53 additions & 0 deletions .github/workflows/reusable-bot-autoassign.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
name: Reusable Bot Autoassign

on:
workflow_call:
inputs:
bot_command:
required: true
type: string
description: "The bot to execute (e.g., 'issue_assignment', 'pr_reopen', 'stale_pr')"
secrets:
OPENWISP_BOT_APP_ID:
required: true
OPENWISP_BOT_PRIVATE_KEY:
required: true

jobs:
run-bot:
runs-on: ubuntu-latest
steps:
- name: Generate GitHub App token
id: generate-token
uses: actions/create-github-app-token@v2
with:
app-id: ${{ secrets.OPENWISP_BOT_APP_ID }}
private-key: ${{ secrets.OPENWISP_BOT_PRIVATE_KEY }}

- name: Checkout openwisp-utils
uses: actions/checkout@v6
with:
repository: openwisp/openwisp-utils
ref: master
path: openwisp-utils

- name: Set up Python
uses: actions/setup-python@v6
with:
python-version: "3.13"

- name: Install dependencies
run: pip install -e 'openwisp-utils/.[github_actions]'

- name: Execute bot script
env:
GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }}
REPOSITORY: ${{ github.repository }}
GITHUB_EVENT_NAME: ${{ github.event_name }}
BOT_COMMAND: ${{ inputs.bot_command }}
run: |
if [ -n "$GITHUB_EVENT_PATH" ]; then
python openwisp-utils/.github/actions/bot-autoassign/__main__.py "$BOT_COMMAND" "$GITHUB_EVENT_PATH"
else
python openwisp-utils/.github/actions/bot-autoassign/__main__.py "$BOT_COMMAND"
fi
Loading
Loading