Create testsuite for GithubClient

[ehuss]

This adds a new testsuite for verifying the behavior for GithubClient. This can assist with adding new functions where you can verify that everything is working correctly without having to guess.

In general this works by setting up a simple HTTP server, sending the requests to it, and verifying the responses. Tests can be created by recording against the live GitHub site, but otherwise tests only run against the local test server.

The first commit updates GithubClient so that it can configure the host it connects to.

[ehuss]

This is just the client side of #1678. I'm not sure when I'll be able to make time to finish up the server tests. Those are more complicated and I'm not as confident in them. The client tests are simpler and I think should be reliable and relatively easy to write and run.

[ehuss]

@Mark-Simulacrum Just checking if you'll have a chance to review this. I think this would help with avoiding issues like #1707 where there are issues in the GithubClient. I realize it adds a lot of noisy files, but I think it is important to have real-world data to test against. When reviewing, I suggest just looking at the filenames to get the gist of what a test is doing and to make sure it seems reasonable (like the remove_label does a DELETE on the labels endpoint). I expect any tests added in the future will be a little easier to review since it will be a smaller set of changes.

I also realize writing these tests takes a little effort. However, I've tried to minimize that as much as possible, along with clear step-by-step directions. (But, to be honest, I'll be surprised if anyone writes their own.)

[Mark-Simulacrum]

I have a few questions, but broadly I'm fine with merging this as-is -- if it gives us pain we can always change course, rather than hashing out all the details up front.

[apiraino]

@ehuss IIUC there's no Rust crate that could provide a satisfactory solution so we're rolling our own, correct?

[apiraino]

I also realize writing these tests takes a little effort. However, I've tried to minimize that as much as possible, along with clear step-by-step directions. (But, to be honest, I'll be surprised if anyone writes their own.)

if you suspect that hardly anyone will write their own tests with the current testsuite drafted here, wouldn't that diminish its usefulness? is there a way to make them more palatable?

(I'm trying to look at this patch through the lens of a possibly new contributor)

EDIT: fixed spelling

[ehuss]

I am trying to understand these changes. What if we want to add other server endpoints - say Zulip ones? Do we need to entirely duplicate ./tests/testsuite.rs and tests/github_client/mod.rs and change bits here and there?

The intent is that tests/testsuite.rs is a base that can be used for recording and mocking services. I haven't looked at mocking Zulip, yet, but I assume it should be relatively easy.

1678 demonstrates how other test suites can be built upon tests/testsuite.rs. tests/server_test/mod.rs is an example of the webhook server tests (that handles triagebot handlers and such).

If there is another suite (like Zulip) that needs something similar to what is in github_client/mod.rs, I figure it can be factored out if necessary. For the server tests, there wasn't anything else that was shareable.

@ehuss IIUC there's no Rust crate that could provide a satisfactory solution so we're rolling our own, correct?

I'm not sure if you mean for testing, or for the github client itself. For testing, I'm not aware of something that does exactly something like this. Octocrab uses wiremock, but I don't see a way to record live site data with that. The original approach in #1678 was to have hand-crafted mocked endpoints, but that takes a lot more work to set up correctly, so that's why I added the automatic recording.

if you suspect that anyone will write their own tests with the current testsuite drafted here, wouldn't that diminish its usefulness? is there a way to make them more palatable?

I'm not sure it would diminish it. It would just mean that new endpoints wouldn't be tested. It would be nice to have people be willing to participate in setting up tests. I was just saying that from the more cynical side of me that knows many people don't like writing tests.

I'd like to make it more palatable, though I'd like some feedback on what specifically looks challenging. I felt like they were pretty easy for me to write (each test took about 5 minutes to write). If you were writing a test, which step seems difficult or something you would be unwilling to do? I tried to make it as easy as I possibly could, but there weren't any more steps I could remove. I think the TRIAGEBOT_TEST_RECORD_DIR thing is cumbersome, and I could look at writing something like an xtask to simplify that command (like cargo xtask-record TESTNAME). Let me know if you have ideas on how to make it better, or what looks particularly troublesome.

[apiraino]

@ehuss IIUC there's no Rust crate that could provide a satisfactory solution so we're rolling our own, correct?

I'm not sure if you mean for testing, or for the github client itself. For testing, I'm not aware of something that does exactly something like this. Octocrab uses wiremock, but I don't see a way to record live site data with that. The original approach in #1678 was to have hand-crafted mocked endpoints, but that takes a lot more work to set up correctly, so that's why I added the automatic recording.

To be honest I just have a feeling that we are reinventing the wheel (and then we have to maintain it). The specific wheel I suspect we are reinventing is cargo-insta but if cargo-insta does not what you want to implement, then let's roll our own :)

@ehuss thanks for your patience and replies to all my questions :)

[ehuss]

@ehuss thanks for your patience and replies to all my questions :)

I neglected to say this earlier, but thank you for the review and feedback! It is useful, and I'll try to hone this to improve it from that. Questions are very much encouraged.

[jackh726]

Hi @ehuss, I'd like to work to get this landed.

The first commit (configure API URLs) seems like it would be good to land on its own. Can you split that out and I'll merge it?

[ehuss]

The first commit (configure API URLs) seems like it would be good to land on its own. Can you split that out and I'll merge it?

Sure, I went ahead and posted #1764.