add fetch retries #124
add fetch retries #124
Conversation
There was a problem hiding this comment.
I just improved the type inference for all these mocks. I was having issues when trying to mock things properly without them.
| repo: { | ||
| owner: "repo-owner", | ||
| name: "repo-name", | ||
| repo: "repo-name", |
There was a problem hiding this comment.
This wasn't mocked correctly, and the tests didn't catch it. I added tests for it now in telemetry.test.ts
| export interface MockResponseBuilder<T extends unknown[], U extends unknown[]> { | ||
| addSuccessfulResponse: (...options: T) => MockResponseBuilder<T, U>; | ||
| addErrorResponse: (...options: U) => MockResponseBuilder<T, U>; | ||
| build: () => { | ||
| handler: HttpHandler; | ||
| mock: jest.Mock<(request: Request) => void>; | ||
| }; | ||
| } | ||
|
|
||
| const mockResponseBuilder = <T extends unknown[], U extends unknown[]>({ |
There was a problem hiding this comment.
This is a little fancy, but it make writing msw mocks so much nicer
| describe("fetchApiAddress", () => { | ||
| const oldEnv = process.env; | ||
|
|
||
| beforeEach(() => { | ||
| jest.resetModules(); | ||
| process.env = oldEnv; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env = oldEnv; | ||
| }); | ||
|
|
||
| it("defaults to prod if there is no address", () => { | ||
| delete process.env.TRUNK_PUBLIC_API_ADDRESS; | ||
| expect(fetchApiAddress()).toBe("https://api.trunk.io"); | ||
| }); | ||
|
|
||
| it("uses the provided address", () => { | ||
| process.env.TRUNK_PUBLIC_API_ADDRESS = "https://myFancyDeploy.trunk.ca"; | ||
| expect(fetchApiAddress()).toBe("https://myFancyDeploy.trunk.ca"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("convertToTelemetry", () => { | ||
| it("falls back to prod when given an invalid address", () => { | ||
| expect(convertToTelemetry("html://notADomain.oops")).toBe( | ||
| "https://telemetry.api.trunk.io/v1/flakytests-uploader/upload-metrics", | ||
| ); | ||
| }); | ||
|
|
||
| it("adapts a prod address", () => { | ||
| expect(convertToTelemetry("https://api.trunk.io")).toBe( | ||
| "https://telemetry.api.trunk.io/v1/flakytests-uploader/upload-metrics", | ||
| ); | ||
| }); | ||
|
|
||
| it("adapts a prod address with an extra slash", () => { | ||
| expect(convertToTelemetry("https://api.trunk.io/")).toBe( | ||
| "https://telemetry.api.trunk.io/v1/flakytests-uploader/upload-metrics", | ||
| ); | ||
| }); | ||
|
|
||
| it("adapts a devenv", () => { | ||
| expect(convertToTelemetry("https://api.dev1.trunk-staging.io")).toBe( | ||
| "https://telemetry.api.dev1.trunk-staging.io/v1/flakytests-uploader/upload-metrics", | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Moved to telemetry.test.ts
There was a problem hiding this comment.
Why the massive decrease in size? I added a minify flag. This should greatly improve the time it takes to download the repo
| "type": "module", | ||
| "scripts": { | ||
| "build": "rm -rf dist && ncc build src/index.ts -o dist", | ||
| "build": "rm -rf dist && ncc build -m src/index.ts -o dist", |
There was a problem hiding this comment.
this is a flag for minifying
| "@jest/globals": "^29.7.0", | ||
| "@vercel/ncc": "^0.38.3", |
There was a problem hiding this comment.
moved to devDependencies
| "promise-retry": "^2.0.1", | ||
| "protobufjs": "^7.5.3", | ||
| "retry": "^0.13.1", | ||
| "ts-protoc-gen": "^0.15.0" |
There was a problem hiding this comment.
exponential-backoffreplacedpromise-retryandretryts-protoc-genwas unused
| "eslint-plugin-jest": "^28.9.0", | ||
| "jest": "^29.7.0", | ||
| "jest-junit": "^16.0.0", | ||
| "protoc-gen-js": "3.21.4-4", |
| "protoc-gen-js": "3.21.4-4", | ||
| "msw": "^2.12.7", | ||
| "ts-jest": "^29.2.5", | ||
| "ts-node": "^10.9.2", |
dfrankland
force-pushed
the
dylan/add-fetch-retries
branch
from
6b9df9f to
7d35426
Compare
| jest.unstable_mockModule("@actions/core", () => core); | ||
| jest.unstable_mockModule("@actions/github", () => github); | ||
|
|
||
| const { sendTelemetry } = await import("../src/telemetry/index.js"); |
There was a problem hiding this comment.
IMO we should only test the interfaces defined in the application. This provides more coverage and realistic testing
There was a problem hiding this comment.
combined into telemetry.test.ts
| telemetryUploadMock.mock.calls.map(async ([request]) => { | ||
| const buffer = await request.arrayBuffer(); | ||
| const message = UploaderUploadMetrics.decode( | ||
| new Uint8Array(buffer), | ||
| ).toJSON(); | ||
| expect(message).toStrictEqual({ | ||
| failed: false, | ||
| repo: { | ||
| owner: github.context.repo.owner, | ||
| name: github.context.repo.repo, | ||
| host: "github.com", | ||
| }, | ||
| uploader_version: expectedUploaderVersion, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
woo, testing payloads!
There was a problem hiding this comment.
Mostly moved from telemetry.ts. I split out protos.ts because we don't need to define the schema during call execution and reusing the schema in tests is needed
2 participants
Testing for fetches is a bit difficult with "normal" mocks since we there's no built-in way of mocking some read-only properties of the
Responseclass. In our case, redirects add a propertyurlwhich cannot be generated without mocking the entire class, butmswalready does this for us and is a better way of mocking in general.