Test the command parsing logic

[bryanburgers]

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.

[pietroalbini]

Unfortunately I didn't get a chance to look at this and I will be really busy for the next few weeks, so it would be great if someone else can review this!

Here are some random notes about the command parsing and the testing:

• In other projects (like highfive) we use pytest instead of the native unittest module. From my experience it's way better and it has more mindshare in the Python community: would you be ok changing the use of unittest with it?
• One thing that the current command parser supports is the ability to add random text in the same line as an homu command, like @bors retry network error. Could you add support for it (if it doesn't work already) and a test?
  • One bad thing about that feature is the current parser then continues parsing the command, just ignoring the unknown words: @bors retry (yielding priority to the rollup) is actually parsed as @bors retry rollup which is not something anyone wants. When you implement the previous note can you add a test to ensure this doesn't happen anymore?

[bryanburgers]

Hey, no problem. Good luck with your exams!

Yeah, pytest is nice. I didn’t realize there was precedent. I’ll switch over.

I’ll see what I can do about the parse. Seems kinda fuzzy what we do and don’t want, so we’ll see.

[pietroalbini]

I’ll see what I can do about the parse. Seems kinda fuzzy what we do and don’t want, so we’ll see.

The behavior I expect and would like is to parse until an invalid word is found, and then stop parsing that line. Dunno if the others have different opinions.

[Mark-Simulacrum]

That behavior seems reasonable. If it's easier then I think going down the triagebot route with "must have terminator" is also fine (e.g., a period) -- it doesn't really hurt us all that much from a user experience point of view

[pietroalbini]

One issue I could see with the terminator is that nobody is used to it with bors, and I'd like for this PR not to change the current workflow.

[bryanburgers]

I agree about no terminators. That would confuse everybody at this point.

Do we want to test current functionality now, and introduce the order change in a future PR? Or make those parser changes now?

[Mark-Simulacrum]

I agree that it's not ideal to have that behavior change, I just intended to say that I don't think it's the end of the world presuming we indicate the change by throwing errors and suggesting to users that they should insert the terminator as appropriate. But if we can do without, great -- bors/homu's syntax is certainly simpler than triagebot's.

[bryanburgers]

• Switched to pytest
• Updated parsing to stop at unknown words

@pietroalbini @Mark-Simulacrum

[pietroalbini]

Won't have time to properly review this for a while, sorry!

By the way, there is another edge case that would be awesome to solve: ignore all commands when the line starts with >, to hopefully catch quoted emails. Example: rust-lang/rust#61540 (comment)

[pietroalbini]

This looks great! Thank you so much!

[pietroalbini]

Merged this early even if my last comment wasn't handled to unblock your other work. Opened an issue to track commands in quotes: #52