Fix errorRequest tests not testing the right thing

[mbg]

The "should override the state.retries property with the options.request.retries properties" test for errorRequest contains a bug. This becomes apparent if we start typing the test values. The RequestError constructor expects an argument of type RequestErrorOptions, which is defined as:

type RequestErrorOptions = {
  response?: OctokitResponse<unknown> | undefined;
  request: RequestOptions;
};

We call the constructor with new RequestError("Internal server error", 500, errorOptions). The errorOptions value is untyped and the inferred type happens to be compatible with RequestErrorOptions, but is subtly incorrect. If we type the request object, the error becomes apparent:

const errorOptions = {
  request: {
    method: "GET" as RequestMethod,
    url: "/issues",
    headers: {},
    retries: 5, // Type Error: Object literal may only specify known properties, and 'retries' does not exist in type 'RequestOptions'
    request: {},
  } satisfies RequestOptions,
};

The correct place for the retries: 5 property would actually be in the (nested) request object of type RequestRequestOptions:

const errorOptions = {
  request: {
    method: "GET" as RequestMethod,
    url: "/issues",
    headers: {},
    request: {
      retries: 5,
    },
  } satisfies RequestOptions,
};

This mistake leads to two follow-up issues.

The first is that errorRequest is called with errorOptions as argument. In #661, I gave the options parameter of the errorRequest function a minimal type of { request: RequestRequestOptions }. It so happens that both the erroneous and correct versions of errorOptions are compatible with this because they have request keys, all properties in RequestRequestOptions are optional by default, and RequestRequestOptions allows arbitrary extra properties of any type.

So, with the retries property in the wrong place and errorRequest willing to accept the RequestErrorOptions value as argument, errorRequest happens to do the right thing because it uses options.request.retries to get the request-specific retries value, but then constructs a RequestError result based on error (via retryRequest):

{
  status: 500,
  request: {
    method: 'GET',
    url: '/issues',
    headers: {},
    retries: 5,
    request: { retries: 5, retryAfter: 1 }
  },
  response: undefined
}

However, this is actually irrelevant, because the test asserts that:

expect(e.request.retries).toBe(5);

So it is only really checking that e (which is based on error) contains the same retries value that's in the wrong place to begin with. If we added:

expect(e.request.retryAfter).toBe(1);

It would fail because e.request.retryAfter is undefined. Therefore, the test is actually only asserting that the thrown RequestError is based on error, and not that errorRequest actually choose the retries value from the request over the one in the state. The correct assertion would be expect(e.request.retries).toBe(5);.

This PR fixes the construction of errorOptions in both this test and the other one for errorRequest. It also fixes the assertions, and adds extra ones to avoid this problem.

We could additionally change the type of options to be Required<EndpointDefaults> since that's what it would always get in practice, but this would then require us to construct a full EndpointDefaults object in the tests. Happy to make this change if we think that is still needed on top of the test fixes.

---

Before the change?

• The test for "should override the state.retries property with the options.request.retries properties" actually only asserts that the thrown RequestError is based on error, and not that errorRequest actually choose the retries value from the request over the one in the state.

After the change?

• The test asserts the correct thing.

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)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[wolfy1339]

Some minor changes, otherwise it looks good.