Skip to content

Commit 3f3ed92

Browse files
committed
Test the command parsing logic
Start adding some tests to Homu by testing the issue comment command-parsing logic. Take `parse_commands` and break it apart into two phases * Parsing phase * Execution phase The parsing phase returns a list of commands with action names (ideally, this would be a Rust enum, but to simulate that, we use action names on a single class) that are returned regardless of the current state. So for example, `@bors retry` will return a `retry` command regardless of the current state of `realtime`. The execution phase then inspects the list of commands and decides what to do with them. So for example, the `retry` command will be skipped if `realtime == False`. This has the positive result of having a parsing phase that has no side-effects, which makes it much easier to test. This can lead to higher confidence that the code works as expected without the high cost of testing in production and possibly disrupting the build flow. This has the negative result of adding a lot of lines of code to achieve command parsing, which we already do successfully without the tests.
1 parent 5ffafdb commit 3f3ed92

File tree

5 files changed

+787
-70
lines changed

5 files changed

+787
-70
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,4 @@
55
/homu.egg-info/
66
/main.db
77
/cache
8+
*.pyc

.travis.yml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,6 @@ python:
66
install:
77
- pip install flake8
88
script:
9-
- flake8 homu
9+
- flake8 homu
10+
- cd homu
11+
- python -m unittest

homu/main.py

Lines changed: 56 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import functools
77
from . import comments
88
from . import utils
9+
from .parse_issue_comment import parse_issue_comment
910
from .auth import verify as verify_auth
1011
from .utils import lazy_debug
1112
import logging
@@ -15,7 +16,6 @@
1516
import sqlite3
1617
import requests
1718
from contextlib import contextmanager
18-
from itertools import chain
1919
from queue import Queue
2020
import os
2121
import sys
@@ -469,28 +469,20 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
469469
my_username,
470470
)
471471

472-
words = list(chain.from_iterable(re.findall(r'\S+', x) for x in body.splitlines() if '@' + my_username in x)) # noqa
473-
if words[1:] == ["are", "you", "still", "there?"] and realtime:
474-
state.add_comment(
475-
":cake: {}\n\n![]({})".format(
476-
random.choice(PORTAL_TURRET_DIALOG), PORTAL_TURRET_IMAGE)
477-
)
478-
for i, word in reversed(list(enumerate(words))):
472+
hooks = []
473+
if 'hooks' in global_cfg:
474+
hooks = list(global_cfg['hooks'].keys())
475+
476+
commands = parse_issue_comment(username, body, sha, my_username, hooks)
477+
478+
for command in commands:
479479
found = True
480-
if word == 'r+' or word.startswith('r='):
480+
if command.action == 'approve':
481481
if not _reviewer_auth_verified():
482482
continue
483483

484-
if not sha and i + 1 < len(words):
485-
cur_sha = sha_or_blank(words[i + 1])
486-
else:
487-
cur_sha = sha
488-
489-
approver = word[len('r='):] if word.startswith('r=') else username
490-
491-
# Ignore "r=me"
492-
if approver == 'me':
493-
continue
484+
approver = command.actor
485+
cur_sha = command.commit
494486

495487
# Ignore WIP PRs
496488
is_wip = False
@@ -582,7 +574,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
582574
)
583575
state.change_labels(LabelEvent.APPROVED)
584576

585-
elif word == 'r-':
577+
elif command.action == 'unapprove':
586578
if not verify_auth(username, repo_label, repo_cfg, state,
587579
AuthState.REVIEWER, realtime, my_username):
588580
continue
@@ -592,14 +584,12 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
592584
if realtime:
593585
state.change_labels(LabelEvent.REJECTED)
594586

595-
elif word.startswith('p='):
587+
elif command.action == 'prioritize':
596588
if not verify_auth(username, repo_label, repo_cfg, state,
597589
AuthState.TRY, realtime, my_username):
598590
continue
599-
try:
600-
pvalue = int(word[len('p='):])
601-
except ValueError:
602-
continue
591+
592+
pvalue = command.priority
603593

604594
if pvalue > global_cfg['max_priority']:
605595
if realtime:
@@ -611,12 +601,12 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
611601
state.priority = pvalue
612602
state.save()
613603

614-
elif word.startswith('delegate='):
604+
elif command.action == 'delegate':
615605
if not verify_auth(username, repo_label, repo_cfg, state,
616606
AuthState.REVIEWER, realtime, my_username):
617607
continue
618608

619-
state.delegate = word[len('delegate='):]
609+
state.delegate = command.delegate_to
620610
state.save()
621611

622612
if realtime:
@@ -625,14 +615,14 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
625615
.format(state.delegate)
626616
)
627617

628-
elif word == 'delegate-':
618+
elif command.action == 'undelegate':
629619
# TODO: why is this a TRY?
630620
if not _try_auth_verified():
631621
continue
632622
state.delegate = ''
633623
state.save()
634624

635-
elif word == 'delegate+':
625+
elif command.action == 'delegate-author':
636626
if not _reviewer_auth_verified():
637627
continue
638628

@@ -645,7 +635,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
645635
.format(state.delegate)
646636
)
647637

648-
elif word == 'retry' and realtime:
638+
elif command.action == 'retry' and realtime:
649639
if not _try_auth_verified():
650640
continue
651641
state.set_status('')
@@ -654,7 +644,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
654644
state.record_retry_log(command_src, body)
655645
state.change_labels(event)
656646

657-
elif word in ['try', 'try-'] and realtime:
647+
elif command.action in ['try', 'untry'] and realtime:
658648
if not _try_auth_verified():
659649
continue
660650
if state.status == '' and state.approved_by:
@@ -665,7 +655,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
665655
)
666656
continue
667657

668-
state.try_ = word == 'try'
658+
state.try_ = command.action == 'try'
669659

670660
state.merge_sha = ''
671661
state.init_build_res([])
@@ -676,14 +666,14 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
676666
# any meaningful labeling events.
677667
state.change_labels(LabelEvent.TRY)
678668

679-
elif word in WORDS_TO_ROLLUP:
669+
elif command.action == 'rollup':
680670
if not _try_auth_verified():
681671
continue
682-
state.rollup = WORDS_TO_ROLLUP[word]
672+
state.rollup = command.rollup_value
683673

684674
state.save()
685675

686-
elif word == 'force' and realtime:
676+
elif command.action == 'force' and realtime:
687677
if not _try_auth_verified():
688678
continue
689679
if 'buildbot' in repo_cfg:
@@ -712,61 +702,58 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username,
712702
':bomb: Buildbot returned an error: `{}`'.format(err)
713703
)
714704

715-
elif word == 'clean' and realtime:
705+
elif command.action == 'clean' and realtime:
716706
if not _try_auth_verified():
717707
continue
718708
state.merge_sha = ''
719709
state.init_build_res([])
720710

721711
state.save()
722-
elif (word == 'hello?' or word == 'ping') and realtime:
723-
state.add_comment(":sleepy: I'm awake I'm awake")
724-
elif word.startswith('treeclosed='):
712+
713+
elif command.action == 'ping' and realtime:
714+
if command.ping_type == 'portal':
715+
state.add_comment(
716+
":cake: {}\n\n![]({})".format(
717+
random.choice(PORTAL_TURRET_DIALOG),
718+
PORTAL_TURRET_IMAGE)
719+
)
720+
else:
721+
state.add_comment(":sleepy: I'm awake I'm awake")
722+
723+
elif command.action == 'treeclosed':
725724
if not _reviewer_auth_verified():
726725
continue
727-
try:
728-
treeclosed = int(word[len('treeclosed='):])
729-
state.change_treeclosed(treeclosed, command_src)
730-
except ValueError:
731-
pass
726+
state.change_treeclosed(command.treeclosed_value, command_src)
732727
state.save()
733-
elif word == 'treeclosed-':
728+
729+
elif command.action == 'untreeclosed':
734730
if not _reviewer_auth_verified():
735731
continue
736732
state.change_treeclosed(-1, None)
737733
state.save()
738-
elif 'hooks' in global_cfg:
739-
hook_found = False
740-
for hook in global_cfg['hooks']:
741-
hook_cfg = global_cfg['hooks'][hook]
742-
if hook_cfg['realtime'] and not realtime:
734+
735+
elif command.action == 'hook':
736+
hook = command.hook_name
737+
hook_cfg = global_cfg['hooks'][hook]
738+
if hook_cfg['realtime'] and not realtime:
739+
continue
740+
if hook_cfg['access'] == "reviewer":
741+
if not _reviewer_auth_verified():
743742
continue
744-
if word == hook or word.startswith('%s=' % hook):
745-
if hook_cfg['access'] == "reviewer":
746-
if not _reviewer_auth_verified():
747-
continue
748-
else:
749-
if not _try_auth_verified():
750-
continue
751-
hook_found = True
752-
extra_data = ""
753-
if word.startswith('%s=' % hook):
754-
extra_data = word.split("=")[1]
755-
Thread(
756-
target=handle_hook_response,
757-
args=[state, hook_cfg, body, extra_data]
758-
).start()
759-
if not hook_found:
760-
found = False
743+
else:
744+
if not _try_auth_verified():
745+
continue
746+
Thread(
747+
target=handle_hook_response,
748+
args=[state, hook_cfg, body, command.hook_extra]
749+
).start()
761750

762751
else:
763752
found = False
764753

765754
if found:
766755
state_changed = True
767756

768-
words[i] = ''
769-
770757
return state_changed
771758

772759

0 commit comments

Comments
 (0)