Create testsuite for GithubClient #1698

ehuss · 2023-06-07T23:37:18Z

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 · 2023-06-07T23:39:02Z

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 · 2023-07-10T22:31:05Z

@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 · 2023-07-12T02:13:53Z

tests/github_client/mod.rs

+//! look like. To write a test, follow these steps:
+//!
+//! 1. Choose a repo in your GitHub account to run tests against. I use either
+//!    `ehuss/rust` or `ehuss/triagebot-test`, but any in your account will do.


Am I right in interpreting this means that if (for example) I adjust the functionality for some test -- passing different parameters, querying slightly different information, etc. -- I have no good way of re-running it without changing lots of irrelevant details in the output? (Since I lack access to your repository for test creation etc.).

I think the tradeoff probably still makes sense - in practice hopefully we can avoid updating these files too much -- but would like to hear your take on that.

a variant of the above comment: is there a way to regenerate the JSON files automatically - in case of said small change? maybe I think no, right? You have to delete all of the relevant JSON test files, make said small change, re-run the tests and try to diff the new JSON generated files?

Yea, regenerating an existing test is not particularly easy. My expectation is that the process would look like:

Make a change to GithubClient.

Edit the test to point to a repo you control (like Mark-Simulacrum/rust).

Re-record the test using TRIAGEBOT_TEST_RECORD_DIR=github_client/TEST_NAME_HERE cargo test --test testsuite -- --exact github_client::TEST_NAME_HERE

Add any new assertions relevant to your change (or fix any errors with the existing assertions).

Unfortunately some of the tests have setup that is required (like creating a PR, creating labels, etc.) that the tests currently don't do (I did that manually). I can look at making those tests do all the setup they need. #1678 does this to some degree, but it requires more code to do the setup.

Regenerating all of the JSON files isn't currently possible. The tests need the ability to make changes on the live GitHub site, which means it needs to have a repo that it can write to, and it doesn't know what that is. Per the comment below, I can look at adding that possibility. But that would end up changing all the JSON files causing a large diff.

Mark-Simulacrum · 2023-07-12T02:15:21Z

tests/github_client/mod.rs

+//!
+//!    ```sh
+//!    TRIAGEBOT_TEST_RECORD_DIR=github_client/TEST_NAME_HERE cargo test \
+//!        --test testsuite -- --exact github_client::TEST_NAME_HERE


How hard would it be to have a mode that runs the full test suite and checks/updates recordings against a repo that we control? e.g., rustbot/rust or similar?

My main worry about this design is that it gives us potentially false confidence if GitHub's APIs change, and it's somewhat painful to actually update when they do (potentially lots of local setup) vs. something that ideally we could run in CI, perhaps post-merge. Especially if we expect that new tests are primarily authored by "authorized" folks, we could have the CI in question be kickable with e.g. bors try or similar.

I can take a look at making it easier to re-bless all of the tests using a single repo.

One concern is that if you just blanket re-record all of the tests, that will cause a very large diff since almost all of the data will change.

Mark-Simulacrum · 2023-07-12T02:17:39Z

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 · 2023-07-12T11:38:25Z

tests/github_client/mod.rs

+//!    **WARNING**: Do not write tests that modify the rust-lang org repos
+//!    like rust-lang/rust. Write those tests against your own fork (like
+//!    `ehuss/rust`). We don't want to pollute the real repos with things like
+//!    test PRs.


I understand that the testsuite is meant to be run against your own git repos, but I don't quite understand the sentence "We don't want to pollute the real repos with things like test PRs." (but maybe it's just me)

I'll add some clarity to that. I was just meaning, we don't want tests making changes or opening PRs on production repos like https://github.com/rust-lang/rust/.

apiraino · 2023-07-12T11:40:04Z

README.md

@@ -115,6 +115,21 @@ You need to sign up for a free account, and also deal with configuring the GitHu
      * Secret: Enter a shared secret (some longish random text)
      * Events: "Send me everything"

+## Tests


@ehuss by reading this README snippet I don't easily understand at a glance how the testsuite is meant to be used and maintained. Is the documentation actually meant to be in tests/github_client/mod.rs as rustdoc's comments?

A few questions ahead. Sorry for the perhaps vague comments 🙂, I'm trying to squint and understand these changes and their mainteinance cost for future other contributors:

What is needed to test a new handler under ./src/handlers (which is something I'm currently doing)? I assume little to nothing if the new handler is just using the same Github API already tested, right?

(echoing the comment from Mark) What is needed to do to update tests? Say, we change a little something in how we interpret Github responses in some structs under ./src/github.rs?

is there a way to regenerate these JSON files automatically?

Is the documentation actually meant to be in tests/github_client/mod.rs as rustdoc's comments?

I'm not quite sure I understand the question. The documentation in README.md is intended as a high-level overview of testing of triagebot, and includes a link to tests/github_client/mod.rs to read more details of that specific test suite. If and when more suites are added, they could be linked here, too.

What is needed to test a new handler under ./src/handlers (which is something I'm currently doing)? I assume little to nothing if the new handler is just using the same Github API already tested, right?

Right, handler testing isn't part of this PR. Testing of handlers is implemented in #1678, but I'm finding them to be even harder to wield than these tests, so I broke out just the github side of things to try to make some progress.

What is needed to do to update tests? Say, we change a little something in how we interpret Github responses in some structs under ./src/github.rs?

As mentioned above. The general process is to point the test at one of your personal repos, and re-record the live data.

is there a way to regenerate these JSON files automatically?

Also mentioned above. I can take a look at making it easier to regenerate them.

apiraino

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?

apiraino · 2023-07-12T11:42:13Z

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

apiraino · 2023-07-12T11:51:31Z

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 · 2023-07-18T02:30:13Z

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 · 2023-07-18T08:58:29Z

@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 · 2023-07-18T14:13:53Z

@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 · 2024-01-20T18:11:18Z

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?

jackh726

A few initial comments

jackh726 · 2024-01-20T18:12:58Z

src/github.rs

@@ -1066,6 +1082,15 @@ impl Repository {
            || filters.iter().any(|&(key, _)| key == "no")
            || is_pr && !include_labels.is_empty();

+        let set_is_pr = |issues: &mut [Issue]| {


This could use a comment!

jackh726 · 2024-01-20T18:13:45Z

src/github.rs

@@ -1562,6 +1589,16 @@ impl Repository {
            })?;
        Ok(())
    }
+
+    pub async fn get_pr(&self, client: &GithubClient, number: u64) -> anyhow::Result<Issue> {


Is this used anywhere?

src/github.rs

ehuss · 2024-01-22T01:05:38Z

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.

ehuss force-pushed the gh-test-client branch from e79b2c7 to 11fc233 Compare June 7, 2023 23:38

ehuss force-pushed the gh-test-client branch 3 times, most recently from 60df51b to a47947b Compare June 21, 2023 02:33

ehuss added 3 commits July 10, 2023 15:03

Make it so that the API URLs are configurable for testing

2b9c820

Create testsuite for GithubClient.

44b0496

Add test for merge_upstream conflict.

9f14128

ehuss force-pushed the gh-test-client branch from a47947b to 9f14128 Compare July 10, 2023 22:04

Mark-Simulacrum reviewed Jul 12, 2023

View reviewed changes

apiraino reviewed Jul 12, 2023

View reviewed changes

jackh726 reviewed Jan 20, 2024

View reviewed changes

ehuss mentioned this pull request Jan 22, 2024

Make it so that the API URLs are configurable for testing #1764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create testsuite for GithubClient #1698

Create testsuite for GithubClient #1698

ehuss commented Jun 7, 2023

ehuss commented Jun 7, 2023

ehuss commented Jul 10, 2023

Mark-Simulacrum Jul 12, 2023

apiraino Jul 12, 2023

ehuss Jul 18, 2023

Mark-Simulacrum Jul 12, 2023

ehuss Jul 18, 2023

Mark-Simulacrum commented Jul 12, 2023

apiraino Jul 12, 2023 •

edited

Loading

ehuss Jul 18, 2023

apiraino Jul 12, 2023

ehuss Jul 18, 2023

apiraino left a comment

apiraino commented Jul 12, 2023

apiraino commented Jul 12, 2023 •

edited

Loading

ehuss commented Jul 18, 2023

apiraino commented Jul 18, 2023

ehuss commented Jul 18, 2023

jackh726 commented Jan 20, 2024

jackh726 left a comment

jackh726 Jan 20, 2024

jackh726 Jan 20, 2024

ehuss commented Jan 22, 2024

Create testsuite for GithubClient #1698

Are you sure you want to change the base?

Create testsuite for GithubClient #1698

Conversation

ehuss commented Jun 7, 2023

ehuss commented Jun 7, 2023

ehuss commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Jul 12, 2023

apiraino Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apiraino left a comment

Choose a reason for hiding this comment

apiraino commented Jul 12, 2023

apiraino commented Jul 12, 2023 • edited Loading

ehuss commented Jul 18, 2023

apiraino commented Jul 18, 2023

ehuss commented Jul 18, 2023

jackh726 commented Jan 20, 2024

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Jan 22, 2024

apiraino Jul 12, 2023 •

edited

Loading

apiraino commented Jul 12, 2023 •

edited

Loading