From 91099657feb90e96bc5f7216b21306bd5c60952f Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Tue, 3 Jan 2023 17:11:28 -0500 Subject: [PATCH 1/7] refactor: move new task's oasst api fetching into OasstApiClient --- website/src/lib/oasst_api_client.ts | 63 +++++++++++++++++++ website/src/pages/api/new_task/[task_type].ts | 33 ++-------- 2 files changed, 68 insertions(+), 28 deletions(-) create mode 100644 website/src/lib/oasst_api_client.ts diff --git a/website/src/lib/oasst_api_client.ts b/website/src/lib/oasst_api_client.ts new file mode 100644 index 00000000..ce61e591 --- /dev/null +++ b/website/src/lib/oasst_api_client.ts @@ -0,0 +1,63 @@ +import { JWT } from "next-auth/jwt"; + +class OasstError { + message: string; + errorCode: number; + httpStatusCode: number; + + constructor(message: string, errorCode: number, httpStatusCode: number) { + this.message = message; + this.errorCode = errorCode; + this.httpStatusCode = httpStatusCode; + } +} + +export default class OasstApiClient { + constructor(private readonly oasstApiUrl: string, private readonly oasstApiKey: string) {} + + private async post(path: string, body: any): Promise { + const resp = await fetch(`${this.oasstApiUrl}${path}`, { + method: "POST", + headers: { + "X-API-Key": this.oasstApiKey, + "Content-Type": "application/json", + }, + body: JSON.stringify(body), + }); + + if (resp.status == 204) { + return null; + } + + if (resp.status >= 300) { + try { + const error = await resp.clone().json(); + throw new OasstError(error.message, error.error_code, resp.status); + } catch (e) { + throw new OasstError(await resp.text(), 0, resp.status); + } + } + + return await resp.json(); + } + + // TODO return a strongly typed Task? + // This method is used to store a task in RegisteredTask.task. + // This is a raw Json type, so we can't use it to strongly type the task. + async fetchTask(taskType: string, userToken: JWT): Promise { + return this.post("/api/v1/tasks/", { + type: taskType, + user: { + id: userToken.sub, + display_name: userToken.name || userToken.email, + auth_method: "local", + }, + }); + } + + async ackTask(taskId: string, messageId: string): Promise { + return this.post(`/api/v1/tasks/${taskId}/ack`, { + message_id: messageId, + }); + } +} diff --git a/website/src/pages/api/new_task/[task_type].ts b/website/src/pages/api/new_task/[task_type].ts index 50f0b4e2..bbe31bef 100644 --- a/website/src/pages/api/new_task/[task_type].ts +++ b/website/src/pages/api/new_task/[task_type].ts @@ -1,4 +1,5 @@ import { getToken } from "next-auth/jwt"; +import OasstApiClient from "src/lib/oasst_api_client"; import prisma from "src/lib/prismadb"; /** @@ -20,25 +21,10 @@ const handler = async (req, res) => { return; } + const oasstApiClient = new OasstApiClient(process.env.FASTAPI_URL, process.env.FASTAPI_KEY); + // Fetch the new task. - // - // This needs to be refactored into an easier to use library. - const taskRes = await fetch(`${process.env.FASTAPI_URL}/api/v1/tasks/`, { - method: "POST", - headers: { - "X-API-Key": process.env.FASTAPI_KEY, - "Content-Type": "application/json", - }, - body: JSON.stringify({ - type: task_type, - user: { - id: token.sub, - display_name: token.name || token.email, - auth_method: "local", - }, - }), - }); - const task = await taskRes.json(); + const task = await oasstApiClient.fetchTask(task_type, token); // Store the task and link it to the user.. const registeredTask = await prisma.registeredTask.create({ @@ -53,16 +39,7 @@ const handler = async (req, res) => { }); // Update the backend with our Task ID - await fetch(`${process.env.FASTAPI_URL}/api/v1/tasks/${task.id}/ack`, { - method: "POST", - headers: { - "X-API-Key": process.env.FASTAPI_KEY, - "Content-Type": "application/json", - }, - body: JSON.stringify({ - message_id: registeredTask.id, - }), - }); + await oasstApiClient.ackTask(task.id, registeredTask.id); // Send the results to the client. res.status(200).json(registeredTask); From b7fb1325b22d23213939f8effa96bdcfe99fc700 Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Tue, 3 Jan 2023 17:12:02 -0500 Subject: [PATCH 2/7] test: add contract tests for fetchTask and ackTask --- .../e2e/oasst_api_contract_tests.cy.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 website/cypress/e2e/oasst_api_contract_tests.cy.ts diff --git a/website/cypress/e2e/oasst_api_contract_tests.cy.ts b/website/cypress/e2e/oasst_api_contract_tests.cy.ts new file mode 100644 index 00000000..94545358 --- /dev/null +++ b/website/cypress/e2e/oasst_api_contract_tests.cy.ts @@ -0,0 +1,24 @@ +import OasstApiClient from "src/lib/oasst_api_client"; + +describe("Contract test for Oasst API", function () { + const oasstApiClient = new OasstApiClient("http://localhost:8080", "test"); + + it("can fetch a task", async () => { + expect( + await oasstApiClient.fetchTask("random", { + sub: "test", + name: "test", + email: "test", + }) + ).to.be.not.null; + }); + + it("can ack a task", async () => { + const task = await oasstApiClient.fetchTask("random", { + sub: "test", + name: "test", + email: "test", + }); + expect(await oasstApiClient.ackTask(task.id, "321")).to.be.null; + }); +}); From 5ed4131720e60c38a4b0a5873f23d1c082d52d9f Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Tue, 3 Jan 2023 20:51:11 -0500 Subject: [PATCH 3/7] ci: run contract tests through separate cypress command and add into CI --- .github/workflows/test-api-contract.yaml | 10 +++++++++- scripts/frontend-development/run-contract-test.sh | 11 +++++++++++ website/cypress.config.contract.js | 9 +++++++++ .../{e2e => contract}/oasst_api_contract_tests.cy.ts | 1 + website/package.json | 1 + 5 files changed, 31 insertions(+), 1 deletion(-) create mode 100755 scripts/frontend-development/run-contract-test.sh create mode 100644 website/cypress.config.contract.js rename website/cypress/{e2e => contract}/oasst_api_contract_tests.cy.ts (93%) diff --git a/.github/workflows/test-api-contract.yaml b/.github/workflows/test-api-contract.yaml index 3707f4de..a541e887 100644 --- a/.github/workflows/test-api-contract.yaml +++ b/.github/workflows/test-api-contract.yaml @@ -15,6 +15,9 @@ jobs: - uses: actions/setup-python@v4 with: python-version: "3.10" + - uses: actions/setup-node@v3 + with: + node-version: 16 - run: cd oasst-shared && pip install -e . @@ -22,9 +25,14 @@ jobs: - run: cd backend && pip install -r requirements.txt + - run: cd frontend && npm install + - run: ./scripts/backend-development/start-mock-server.sh - - name: Run contract tests + - name: Run Python OasstApiClient contract tests run: ./scripts/oasst-shared-development/test.sh + - name: Run JavaScript OasstApiClient contract tests + run: ./scripts/frontend-development/run-contract-test.sh + - run: ./scripts/backend-development/stop-mock-server.sh diff --git a/scripts/frontend-development/run-contract-test.sh b/scripts/frontend-development/run-contract-test.sh new file mode 100755 index 00000000..6bedc903 --- /dev/null +++ b/scripts/frontend-development/run-contract-test.sh @@ -0,0 +1,11 @@ +#!/usr/bin/env bash +parent_path=$( cd "$(dirname "${BASH_SOURCE[0]}")" ; pwd -P ) + +# switch to website directory +pushd "$parent_path/../../website" + +set -xe + +npm run cypress:run:contract + +popd diff --git a/website/cypress.config.contract.js b/website/cypress.config.contract.js new file mode 100644 index 00000000..f4461158 --- /dev/null +++ b/website/cypress.config.contract.js @@ -0,0 +1,9 @@ +import { defineConfig } from "cypress"; + +export default defineConfig({ + e2e: { + // No baseUrl here, because we don't need it for contract testing + baseUrl: null, + specPattern: "cypress/contract/*.cy.{ts,js}", + }, +}); diff --git a/website/cypress/e2e/oasst_api_contract_tests.cy.ts b/website/cypress/contract/oasst_api_contract_tests.cy.ts similarity index 93% rename from website/cypress/e2e/oasst_api_contract_tests.cy.ts rename to website/cypress/contract/oasst_api_contract_tests.cy.ts index 94545358..2570acec 100644 --- a/website/cypress/e2e/oasst_api_contract_tests.cy.ts +++ b/website/cypress/contract/oasst_api_contract_tests.cy.ts @@ -1,6 +1,7 @@ import OasstApiClient from "src/lib/oasst_api_client"; describe("Contract test for Oasst API", function () { + // Assumes this is running the mock server. const oasstApiClient = new OasstApiClient("http://localhost:8080", "test"); it("can fetch a task", async () => { diff --git a/website/package.json b/website/package.json index c1d0c3d2..e5240727 100644 --- a/website/package.json +++ b/website/package.json @@ -12,6 +12,7 @@ "build-storybook": "build-storybook", "cypress": "cypress open", "cypress:run": "cypress run", + "cypress:run:contract": "cypress run --config-file ./cypress.config.contract.js", "cypress:image-baseline": "cypress-image-diff -u", "fix:lint": "eslint --fix src/ --ext .js,.jsx,.ts,.tsx", "fix:format": "prettier --write ./src", From 7b5f702a0013dd140953f24189f69a6d84ef9593 Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Tue, 3 Jan 2023 20:54:49 -0500 Subject: [PATCH 4/7] docs: add TODOs --- website/cypress/contract/oasst_api_contract_tests.cy.ts | 4 ++++ website/src/pages/api/update_task.ts | 1 + 2 files changed, 5 insertions(+) diff --git a/website/cypress/contract/oasst_api_contract_tests.cy.ts b/website/cypress/contract/oasst_api_contract_tests.cy.ts index 2570acec..0f9ddd00 100644 --- a/website/cypress/contract/oasst_api_contract_tests.cy.ts +++ b/website/cypress/contract/oasst_api_contract_tests.cy.ts @@ -22,4 +22,8 @@ describe("Contract test for Oasst API", function () { }); expect(await oasstApiClient.ackTask(task.id, "321")).to.be.null; }); + + // TODO Add test for 204 + // TODO Add test for parsing >=300, throwing an OasstError + // TODO Add test for parsing >=300, throwing a generic error }); diff --git a/website/src/pages/api/update_task.ts b/website/src/pages/api/update_task.ts index 9582040b..5e887175 100644 --- a/website/src/pages/api/update_task.ts +++ b/website/src/pages/api/update_task.ts @@ -36,6 +36,7 @@ const handler = async (req, res) => { // Send the interaction to the Task Backend. This automatically fetches the // next task in the sequence (or the done task). + // TODO Move this into OasstApiClient. const interactionRes = await fetch(`${process.env.FASTAPI_URL}/api/v1/tasks/interaction`, { method: "POST", headers: { From a95e71d6f9b8c46d8f6d875cb1cce12bfd549e63 Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Tue, 3 Jan 2023 20:59:37 -0500 Subject: [PATCH 5/7] ci: cd into correct directory to install deps --- .github/workflows/test-api-contract.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-api-contract.yaml b/.github/workflows/test-api-contract.yaml index a541e887..4ca36da0 100644 --- a/.github/workflows/test-api-contract.yaml +++ b/.github/workflows/test-api-contract.yaml @@ -25,7 +25,7 @@ jobs: - run: cd backend && pip install -r requirements.txt - - run: cd frontend && npm install + - run: cd website && npm install - run: ./scripts/backend-development/start-mock-server.sh From bdcfae54fc1ed5edd8d8c7235a859e169e65a8a9 Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Wed, 4 Jan 2023 08:04:27 -0500 Subject: [PATCH 6/7] docs: update TODOs to include issue numbers --- website/cypress/contract/oasst_api_contract_tests.cy.ts | 6 +++--- website/src/pages/api/update_task.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/website/cypress/contract/oasst_api_contract_tests.cy.ts b/website/cypress/contract/oasst_api_contract_tests.cy.ts index 0f9ddd00..ff5bb156 100644 --- a/website/cypress/contract/oasst_api_contract_tests.cy.ts +++ b/website/cypress/contract/oasst_api_contract_tests.cy.ts @@ -23,7 +23,7 @@ describe("Contract test for Oasst API", function () { expect(await oasstApiClient.ackTask(task.id, "321")).to.be.null; }); - // TODO Add test for 204 - // TODO Add test for parsing >=300, throwing an OasstError - // TODO Add test for parsing >=300, throwing a generic error + // TODO(#354): Add test for 204 + // TODO(#354): Add test for parsing >=300, throwing an OasstError + // TODO(#354): Add test for parsing >=300, throwing a generic error }); diff --git a/website/src/pages/api/update_task.ts b/website/src/pages/api/update_task.ts index 5e887175..2d371354 100644 --- a/website/src/pages/api/update_task.ts +++ b/website/src/pages/api/update_task.ts @@ -36,7 +36,7 @@ const handler = async (req, res) => { // Send the interaction to the Task Backend. This automatically fetches the // next task in the sequence (or the done task). - // TODO Move this into OasstApiClient. + // TODO(#353): Move this into OasstApiClient. const interactionRes = await fetch(`${process.env.FASTAPI_URL}/api/v1/tasks/interaction`, { method: "POST", headers: { From 695e1f3932837a49190568ecc10eacf5315fa57f Mon Sep 17 00:00:00 2001 From: Jack Michaud Date: Wed, 4 Jan 2023 14:17:02 -0500 Subject: [PATCH 7/7] refactor: avoid using Response.clone() --- website/src/lib/oasst_api_client.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/website/src/lib/oasst_api_client.ts b/website/src/lib/oasst_api_client.ts index ce61e591..45a0859e 100644 --- a/website/src/lib/oasst_api_client.ts +++ b/website/src/lib/oasst_api_client.ts @@ -30,11 +30,12 @@ export default class OasstApiClient { } if (resp.status >= 300) { + const errorText = await resp.text(); try { - const error = await resp.clone().json(); + const error = JSON.parse(errorText); throw new OasstError(error.message, error.error_code, resp.status); } catch (e) { - throw new OasstError(await resp.text(), 0, resp.status); + throw new OasstError(errorText, 0, resp.status); } }