Skip to content
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.

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

Resolver abs reply to comment #6

Open
wants to merge 2 commits into
base: resolver_abs
Choose a base branch
from
Open
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
38 changes: 38 additions & 0 deletions openhands/resolver/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ def get_download_url(self):
def get_clone_url(self):
pass

@abstractmethod
def get_graphql_url(self):
pass

@abstractmethod
def get_headers(self):
pass
Expand All @@ -47,6 +51,10 @@ def get_compare_url(self, branch_name):
def get_branch_name(self, base_branch_name: str, headers: dict):
pass

@abstractmethod
def reply_to_comment(self, token: str, comment_id: str, reply: str):
pass

@abstractmethod
def branch_exists(self, branch_name: str, headers: dict) -> bool:
pass
Expand Down Expand Up @@ -93,6 +101,9 @@ def get_clone_url(self):
)
return f'https://{username_and_token}@github.com/{self.owner}/{self.repo}.git'

def get_graphql_url(self):
return self.get_base_url() + '/graphql'

def get_compare_url(self, branch_name: str):
return f'https://github.com/{self.owner}/{self.repo}/compare/{branch_name}?expand=1'

Expand Down Expand Up @@ -174,6 +185,33 @@ def get_branch_name(self, base_branch_name: str, headers: dict):
branch_name = f'{base_branch_name}-try{attempt}'
return branch_name

def reply_to_comment(self, token: str, comment_id: str, reply: str):
# Opting for graphql as REST API doesn't allow reply to replies in comment threads
query = """
mutation($body: String!, $pullRequestReviewThreadId: ID!) {
addPullRequestReviewThreadReply(input: { body: $body, pullRequestReviewThreadId: $pullRequestReviewThreadId }) {
comment {
id
body
createdAt
}
}
}
"""

comment_reply = f'Openhands fix success summary\n\n\n{reply}'
variables = {'body': comment_reply, 'pullRequestReviewThreadId': comment_id}
url = self.get_base_url()
headers = {
'Authorization': f'Bearer {token}',
'Content-Type': 'application/json',
}

response = requests.post(
url, json={'query': query, 'variables': variables}, headers=headers
)
response.raise_for_status()

def get_pull_url(self, pr_number: int):
return f'https://github.com/{self.owner}/{self.repo}/pull/{pr_number}'

Expand Down
6 changes: 6 additions & 0 deletions openhands/resolver/issue_definitions.py
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,9 @@ def get_download_url(self):
def get_clone_url(self):
return self._strategy.get_clone_url()

def get_graphql_url(self):
return self._strategy.get_graphql_url()

def get_headers(self):
return self._strategy.get_headers()

Expand All @@ -351,6 +354,9 @@ def get_branch_name(
):
return self._strategy.get_branch_name(base_branch_name, headers)

def reply_to_comment(self, token, comment_id, reply):
return self._strategy.reply_to_comment(token, comment_id, reply)

def branch_exists(self, branch_name: str, headers: dict):
return self._strategy.branch_exists(branch_name, headers)

Expand Down
30 changes: 1 addition & 29 deletions openhands/resolver/send_pull_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,34 +292,6 @@ def send_pull_request(
return url


def reply_to_comment(token: str, comment_id: str, reply: str):
# Opting for graphql as REST API doesn't allow reply to replies in comment threads
query = """
mutation($body: String!, $pullRequestReviewThreadId: ID!) {
addPullRequestReviewThreadReply(input: { body: $body, pullRequestReviewThreadId: $pullRequestReviewThreadId }) {
comment {
id
body
createdAt
}
}
}
"""

comment_reply = f'Openhands fix success summary\n\n\n{reply}'
variables = {'body': comment_reply, 'pullRequestReviewThreadId': comment_id}
url = 'https://api.github.com/graphql'
headers = {
'Authorization': f'Bearer {token}',
'Content-Type': 'application/json',
}

response = requests.post(
url, json={'query': query, 'variables': variables}, headers=headers
)
response.raise_for_status()


def update_existing_pull_request(
issue: Issue,
token: str,
Expand Down Expand Up @@ -417,7 +389,7 @@ def update_existing_pull_request(
explanations = json.loads(additional_message)
for count, reply_comment in enumerate(explanations):
comment_id = issue.thread_ids[count]
reply_to_comment(token, comment_id, reply_comment)
handler.reply_to_comment(token, comment_id, reply_comment)

return pr_url

Expand Down
63 changes: 40 additions & 23 deletions tests/unit/resolver/test_send_pull_request.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

import os
import tempfile
from unittest.mock import MagicMock, call, patch
Expand All @@ -6,6 +7,7 @@

from openhands.core.config import LLMConfig
from openhands.resolver.issue import ReviewThread
from openhands.resolver.github import GithubIssueHandler
from openhands.resolver.resolver_output import Issue, ResolverOutput
from openhands.resolver.send_pull_request import (
apply_patch,
Expand All @@ -14,7 +16,6 @@
make_commit,
process_all_successful_issues,
process_single_issue,
reply_to_comment,
send_pull_request,
update_existing_pull_request,
)
Expand Down Expand Up @@ -241,7 +242,7 @@ def test_initialize_repo(mock_output_dir):
assert f.read() == 'hello world'


@patch('openhands.resolver.send_pull_request.reply_to_comment')
@patch('openhands.resolver.github.GithubIssueHandler.reply_to_comment')
@patch('requests.post')
@patch('subprocess.run')
def test_update_existing_pull_request(
Expand Down Expand Up @@ -557,6 +558,14 @@ def test_reply_to_comment(mock_post):
comment_id = 'test_comment_id'
reply = 'This is a test reply.'

# Create an instance of GithubIssueHandler
handler = GithubIssueHandler(
owner='test-owner',
repo='test-repo',
token=token,
username='test-user'
)

# Mock the response from the GraphQL API
mock_response = MagicMock()
mock_response.status_code = 200
Expand All @@ -575,37 +584,45 @@ def test_reply_to_comment(mock_post):
mock_post.return_value = mock_response

# Act: call the function
reply_to_comment(token, comment_id, reply)
handler.reply_to_comment(token, comment_id, reply)

# Assert: check that the POST request was made with the correct parameters
query = """
mutation($body: String!, $pullRequestReviewThreadId: ID!) {
addPullRequestReviewThreadReply(input: { body: $body, pullRequestReviewThreadId: $pullRequestReviewThreadId }) {
comment {
id
body
createdAt
}
# Assert: check that the POST request was made with the correct parameters
expected_query = """
mutation($body: String!, $pullRequestReviewThreadId: ID!) {
addPullRequestReviewThreadReply(input: { body: $body, pullRequestReviewThreadId: $pullRequestReviewThreadId }) {
comment {
id
body
createdAt
}
}
"""
}
"""

expected_variables = {
# Normalize whitespace in both expected and actual queries
def normalize_whitespace(s):
return ' '.join(s.split())

# Check if the normalized actual query matches the normalized expected query
mock_post.assert_called_once()
actual_call = mock_post.call_args
actual_query = actual_call[1]['json']['query']

assert normalize_whitespace(actual_query) == normalize_whitespace(expected_query)

# Check the variables and headers separately
assert actual_call[1]['json']['variables'] == {
'body': 'Openhands fix success summary\n\n\nThis is a test reply.',
'pullRequestReviewThreadId': comment_id,
}

# Check that the correct request was made to the API
mock_post.assert_called_once_with(
'https://api.github.com/graphql',
json={'query': query, 'variables': expected_variables},
headers={
'Authorization': f'Bearer {token}',
'Content-Type': 'application/json',
},
)
assert actual_call[1]['headers'] == {
'Authorization': f'Bearer {token}',
'Content-Type': 'application/json',
}

# Check that the response status was checked (via response.raise_for_status)
# Check that the response status was checked
mock_response.raise_for_status.assert_called_once()


Expand Down