Use a manual parser for `@rust-timer` commands #1994

Kobzol · 2024-10-11T22:48:17Z

Instead of a regex. This allows the user to pass the arguments in an arbitrary order.

lqd

Left a few comments.

Here's a free test shuffling the arg order.

#[test]
fn queue_command_parameter_order() {
    insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=3 exclude=c,a include=b"),
        @r###"Some(Ok(QueueCommand { include: Some("b"), exclude: Some("c,a"), runs: Some(3) }))"###);
}

site/src/request_handlers/github.rs

lqd · 2024-10-12T09:14:12Z

site/Cargo.toml

@@ -57,3 +57,6 @@ jemalloc-ctl = "0.5"
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 toml = "0.7"
+
+[dev-dependencies]
+insta = "1.40.0"


do we want/need insta? Managing the snapshots files can be cumbersome, and it's yet another tool to use and learn. It's fine if we want to make good use of it in the future, and not a fancy assert_eq!(format!("{obj:?}"), "Struct { field: 'bla'} ").

Since I'm refactoring this, I want to do it properly. Note that I'm not using snapshot files, I'm using the inline snapshots, which makes this much easier to grok, I think.

I'm already anticipating future changes (like your PR to add the backends) parameter. It's no fun updating tens of tests anytime you change the structure of the thing that you parse. Snapshot testing makes this much easier.

I will document in readme what to do to update the snapshots though.

Insta adds snapshot files locally when a test fails, the pending snap file. I saw that when adding the test I shared. It didn’t remove it when the test passed.

Ah, these things. Right, I guess we can add them to .gitignore, but they should be removed after you use cargo insta review I think (the snapshots shouldn't really be modified manually).

I didn't have insta installed to do a cargo insta review anyways. We're testing a struct with 3 fields, we don't really need all this test infra imho.

There are already 23 tests in the file, I ain't gonna rewrite all the asserts to make sure we parse exactly what we think we do, every time we change the parsing code 😆 insta was already super helpful for me when developing this PR.

site/src/request_handlers/github.rs

lqd · 2024-10-12T09:22:22Z

site/src/request_handlers/github.rs

+            .map(|index| line[index + prefix.len()..].trim())
+    })?;
+
+    let args = bot_line.strip_prefix("queue").map(|l| l.trim())?;


GH never reformats comments, right? That is, it will not introduce unexpected \ns that would separate the command from some of its arguments?

Uhh, that would be quite surprising I think. GH actually has a bunch of formats of the comments, like text, HTML, MD etc., but I think (hope) that here we work with the raw text. We were already requiring = to be right after include etc. before, so I think that this should be fine.

site/src/request_handlers/github.rs

lqd · 2024-10-12T09:33:41Z

site/src/request_handlers/github.rs

+                format!("Error occurred while parsing comment: {error}")
+            }
+        };
+        main_client.post_comment(issue.number, msg).await;


do we need to take special care (like sanitization) in posting the error message to GH as its text will be partially controlled by users' input commands?

That's an interesting idea. I don't think so, it should have the same rules as when you create the comment manually. I guess that the user could make the bot print "Unknown command argument SEND MONEY TO THE FOLLOWING BITCOIN ADDRESS TO SPONSOR THE RUST FOUNDATION", but I don't think that they can XSS or something like that (that would be really bad hole in GH's security).

lqd · 2024-10-12T09:37:36Z

When we do basically this for the build command it should also fix the bug I mentioned with the regex handling that ignores some of the commands in the comment. I believe I saw it happen during triage where people can want to build multiple shas to analyze a rollup, and "it didn't work".

Kobzol · 2024-10-12T12:31:51Z

I wanted to also refactor the build command parsing, but it's a bit tricky, because it has a positional argument, and I need to move on to other stuff atm :( So let's merge this to make it slightly easier to add the backends parameter. We don't need to support backends in build I think, I haven't ever seen someone use include/exclude with build anyway. Well, I haven't ever seen anyone use it with queue either, but at least we have a use-case for that now 😆

lqd · 2024-10-12T20:09:42Z

Yeah, I've only used include a small number of times in the past. I was mostly thinking of fixing the multiple queue commands bug rather than making my life easier to parse backends, it wasn't that annoying to expand the regexes.

Kobzol · 2024-10-13T07:18:10Z

Decided to go all the way and reimplemented also the build command :) Let me know what do you think.

lqd

I haven't tried super hard to find edge cases in the parsing because the params are never used but this LGTM.

site/src/request_handlers/github.rs

Kobzol · 2024-10-14T13:39:35Z

Well, there is (at least) one regression, the new parser does not really support trailing text, so this fails now:

@rust-timer build sha1234 (PR1234)
@rust-timer build sha1234 (PR1235)

I'm not sure if we want to support that though, it's not trivial to figure out what is a part of the command and what isn't.

lqd · 2024-10-14T13:42:01Z

Yeah I noticed the test change there, but I'm not sure this is used in practice?

Kobzol · 2024-10-14T13:42:23Z

Haven't really seen it IIRC.

lqd · 2024-10-14T13:54:14Z

If there's any issue (esp since we can't really test the GH interactions), we'll debug it in production like everyone does 🙃

Kobzol requested a review from lqd October 11, 2024 22:48

Use a manual parser for rust-timer queue

c7d14cb

Instead of a regex. This allows the user to pass the arguments in an arbitrary order.

Kobzol force-pushed the cmd-parsing-manual branch from d852d3a to c7d14cb Compare October 12, 2024 07:54

lqd approved these changes Oct 12, 2024

View reviewed changes

Kobzol added 2 commits October 12, 2024 14:04

Document our usage of insta

1c0968b

Add test and refactor parsing slightly

bb5711d

Kobzol added 2 commits October 13, 2024 08:49

Refactor queue parsing

b660902

Refactor build command parsing

f3bcb8b

lqd approved these changes Oct 14, 2024

View reviewed changes

site/src/request_handlers/github.rs Show resolved Hide resolved

site/src/request_handlers/github.rs Show resolved Hide resolved

site/src/request_handlers/github.rs Show resolved Hide resolved

site/src/request_handlers/github.rs Show resolved Hide resolved

Kobzol changed the title ~~Use a manual parser for rust-timer queue~~ Use a manual parser for @rust-timer commands Oct 14, 2024

Kobzol merged commit e19c968 into rust-lang:master Oct 14, 2024
11 checks passed

Kobzol deleted the cmd-parsing-manual branch October 14, 2024 16:04

Kobzol mentioned this pull request Oct 14, 2024

Apply forgotten review changes to command parsing #1998

Merged

Use a manual parser for @rust-timer commands #1994

Use a manual parser for @rust-timer commands #1994

Uh oh!

Conversation

Kobzol commented Oct 11, 2024

Uh oh!

lqd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqd commented Oct 12, 2024

Uh oh!

Kobzol commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lqd commented Oct 12, 2024

Uh oh!

Kobzol commented Oct 13, 2024

Uh oh!

lqd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kobzol commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lqd commented Oct 14, 2024

Uh oh!

Kobzol commented Oct 14, 2024

Uh oh!

lqd commented Oct 14, 2024

Uh oh!

Uh oh!

Uh oh!

Use a manual parser for `@rust-timer` commands #1994

Use a manual parser for `@rust-timer` commands #1994

Kobzol commented Oct 12, 2024 •

edited

Loading

Kobzol commented Oct 14, 2024 •

edited

Loading