Add testing for the strings in the actual error message that propagates from Octokit::TooManyRequests

[arjitj2]

Resolves #1590

---

Behavior

Before the change?

We were just checking that Octokit::TooManyRequests was being raised

After the change?

Now we also check that the error message raised contains a specific regex from Github

Other information

---

Additional info

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Added the appropriate label for the given change it won't let me

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes (Please add the Type: Breaking change label)
• No

Pull request type

it won't let me add a label, sorry

---

[nickfloyd]

Hey @arjitj2,

Thanks for being part of the community here! I definitely get where you were headed here, but I don't think the change adds value because:

1. The stub's body covers us on helping us understand why Octokit::TooManyRequests was assigned to the result
2. The match happens in the error class when the 403 is thrown, and the body is evaluated.

We're trying to test if the error class returns the proper error type and nothing else. In effect, we are already verifying the match here, and the passing test affirms that.

Let me know if I missed your intent or if you have any other discussion points on this, I'd be glad to chat about it - but right now it does not appear to add value as far as structural code goes. Again, thank you for taking the time and working to make things better for the community.

[arjitj2]

Hi @nickfloyd , thanks for your response. The goal of this PR is to make sure that Octokit does not hide the error message that led to the Octokit::TooManyRequests, and to verify that the users of Octokit can differentiate between the two conditions.

The stub in the test stubs the response from Github, which makes sense.

      stub_get('/users/mojomobo').to_return \
        status: 403,
        headers: {
          content_type: 'application/json'
        },
        body: { message: 'You have exceeded a secondary rate limit.' }.to_json

We are trying to affirm that the error message transparently passes through from Octokit. The added change to the assert verifies this.

Without the assertion, I had no way of knowing whether or not our Octokit usage would be able to differentiate whether we're running into primary or secondary rate limits.

[nickfloyd]

@arjitj2 Got it. Thanks for the clarity and the work here ❤️