Add integration testing w/external NextJS app (Edge runtime) #304

aulorbe · 2024-10-16T21:52:44Z

Problem

We do not currently have a good way to test how our client behaves end to end in different environments. One of the chief problems our users have brought to our attention in the past is that some functionalities do not work in certain runtimes, e.g. Edge or Bun, and/or with certain frameworks, e.g. NextJS.

Solution

Build an external application in a runtime and framework known to have caused problems in the past and test our client from the end-user's perspective.

This PR introduces and end-to-end test suite that interacts with an external application written using the NextJS framework and the Edge runtime.

Note: the app is automagically run in Edge by way of it using middleware (middleware.ts), and the Headers() API.

Overview of changes

We now have a public repo that contains a super simple application that builds a Pinecone serverless index, seeds it with data, and queries that data. We plan to add more operations in the near future; this is just a start.
This sample application creates an API endpoint that our client repo can send a POST request to and assert on the response. If the endpoint fails to deliver the expected response, we know that something is wrong with the most recent changes we are proposing to introduce in the client.
This PR enables 2 types of interaction with the sample application:
- Local end-to-end tests: for local runs, there is a new bash script that devs can execute. This will spin up the application on their localhost:3000.
- CI end-to-end tests: for CI runs, we will now run this test automatically by way of adding it the testing.yml file that is run for each push to an open PR. In CI, the Github workflow and action hit the application's Vercel endpoint and assert on its response.
There is a new CONTRIBUTING.md file with some info on this new test suite + the other existing ones, as well as a new README in the external-app dir itself with more in depth information.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Infrastructure change (CI configs, etc)
Non-code change (docs, etc)
None of the above: (explain here)

Test Plan

If reviewers can pull down the code in this branch and try to run the tests locally, that'd be great.

To Dos

I'll add a README and a CONTRIBUTING file to the external app

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1208129226784956

.github/actions/e2e-testing/edge/action.yml

aulorbe · 2024-10-18T19:42:45Z

.github/actions/e2e-testing/edge/action.yml

@@ -27,27 +31,68 @@ runs:
      uses: GuillaumeFalourd/clone-github-repo-action@main


Just a note for the previous step: npm pack packages up the existing code on the existing branch, so it'll run the code devs are proposing to merge in their PR, rather than whatever code is on npm etc. This removes the need for us to merge things before testing them end to end.

aulorbe · 2024-10-18T19:43:06Z

.github/actions/e2e-testing/edge/action.yml

+          exit 1
+        fi
+
+        if [ "$match_count" -lt 1 ]; then


This is where we assert on the response in CI

aulorbe · 2024-10-18T19:44:21Z

.github/actions/e2e-testing/edge/action.yml

+
+        response=$(curl --silent --location --request POST "$API_URL" --header "PINECONE_API_KEY: $API_KEY")
+
+        indexName=$(echo "$response" | jq -r '.indexName')


Index names are randomized in the app. We then save the index name to the GITHUB_ENV environment variable file, so that we can delete them later -- this ensures we can have simultaneous runs of this workflow, from creating the index to deleting it later

Would it be possible to port the logic for the actual testing to an explicit TypeScript / Javascript file that handles the calling / asserting, etc?

Something similar to IntegrationRunner.js. I think this will generally be easier to build out and manage if it lives in a code-based script rather than in a GH actions file.

Definitely! Done. Now we have a file called src/end-to-end/localRuns.ts

aulorbe · 2024-10-18T20:02:49Z

src/end-to-end/edge-end-to-end.sh

+# Hop into ts-client-e2e-tests repo, install deps, link local ts-client repo, and start the Next.js server
+pushd "$dir/ts-client-e2e-tests"
+  npm install
+  npm link @pinecone-database/pinecone


This will overwrite the portion in the app's package.json that says "@pinecone-database/pinecone": "^3.0.4-rc.2024-10.0". Instead, it'll link to your local version.

You must run it after npm install, or else npm install will change package.json back to "^3.0.4-rc.2024-10.0", since that's the latest version up on npm as of this writing.

aulorbe · 2024-10-18T20:04:15Z

src/end-to-end/edge-end-to-end.sh

+  next dev
+popd
+
+#npm uninstall -g next # Can comment out if do not want to uninstall the global Next.js package


We do have to install NextJS globally or else the next dev command (to spin up the app on your localhost:3000 won't work when run from the ts-client repo, which is where this bash file is being executed. If you don't want Next globally, you can uncomment line 54 and uninstall it.

I'd maybe add a comment to the script here stating the same thing.

which script?

austin-denoble

Nice work getting this stood up, a lot of moving pieces here. I've left a few questions but we should maybe sync on this so you can walk me through some of the details. I feel like there's a couple things I'm confused about, but it's probably just lack of knowledge around some of this stuff.

austin-denoble · 2024-10-28T16:31:37Z

.github/actions/e2e-testing/edge/action.yml

+
+        response=$(curl --silent --location --request POST "$API_URL" --header "PINECONE_API_KEY: $API_KEY")
+
+        indexName=$(echo "$response" | jq -r '.indexName')


Would it be possible to port the logic for the actual testing to an explicit TypeScript / Javascript file that handles the calling / asserting, etc?

Something similar to IntegrationRunner.js. I think this will generally be easier to build out and manage if it lives in a code-based script rather than in a GH actions file.

austin-denoble · 2024-10-28T16:37:48Z

CONTRIBUTING.md

+Simply run `npm run test:end-to-end -- <clone y/n> <path>` to run all end-to-end tests, passing in `y` or `n` for
+whether you want to clone the repo that contains the external app used for testing, and the `path` the repo should be
+cloned in, or is already cloned in, e.g.:


I'm not necessarily clear on why we there's an option to clone or not clone the test app repo. What's the difference here?

I would think we would just clone it every time to make sure we're running against whatever is fresh there.

Oh that's true, yes.... Okay I'll change!

austin-denoble · 2024-10-28T16:43:33Z

src/end-to-end/edge-end-to-end.sh

+# Recommendation: Do not clone the ts-client-e2e-tests repo in local ts-client repo; it'll just cause npm deps issues
+
+# inputs
+clone=$1 # required -- `y` or `n` to clone the ts-client-e2e-tests repo


This feels a little confusing considering the script has an option to clone or not clone. I asked a question about this previously but I feel like I'm missing something.

austin-denoble · 2024-10-28T16:44:49Z

src/end-to-end/edge-end-to-end.sh

+  next dev
+popd
+
+#npm uninstall -g next # Can comment out if do not want to uninstall the global Next.js package


I'd maybe add a comment to the script here stating the same thing.

austin-denoble · 2024-10-28T16:45:12Z

.github/workflows/testing.yml

+  end-to-end:
+    name: end-to-end
+    runs-on: ubuntu-latest
+    if: always() # Run teardown even if the tests fail


nit: This isn't a teardown job though, right?

Ah must've been a c&p error, apologies! I'll remove.

Ah, no, leaving this here, actually -- the e2e tests include a teardown job, so even if the test fails, the created index will be torn down, ensuring we don't just create orphan indexes in the event that these tests fail.

jhamon

Seems like a solid start. There's a lot of moving pieces to this, so nice job.

jhamon · 2024-10-28T18:53:35Z

.github/actions/e2e-testing/edge/action.yml

      shell: bash

-    - name: Deploy Project Artifacts to Vercel
+    - name: Clean up e2e indexes


This doesn't need to block merging, but I would recommend moving cleanup steps into a different action/job. That lets you retry just the cleanup part, if needed. And easily see that the core of the test is passing even when cleanup steps fail. In our python suite, a ton of our flakes were caused by delete requests failing for various reasons.

Similar example of this in python tests:
https://github.com/pinecone-io/pinecone-python-client/blob/main/.github/workflows/testing-dependency.yaml#L171

I'll make a fast-follow ticket!

jhamon · 2024-10-28T18:56:54Z

package.json

@@ -26,6 +26,7 @@
    "test:integration:edge": "TEST_ENV=edge jest src/integration/ -c jest.config.integration-edge.js --runInBand --bail",
    "test:integration-local:edge": "TEST_ENV=edge node src/integration/integrationRunner.js",
    "test:integration:cleanup": "npm run build && node utils/cleanupResources.ts",
+    "test:end-to-end": "chmod +x ./src/end-to-end/edge-end-to-end.sh && ./src/end-to-end/edge-end-to-end.sh $npm_config_argv",


nit: Seems like a different command name could give better context on what these tests are. All integration tests are "end to end", so it's not obvious from this name how your new suite differs from things like test:integration:edge above.

jhamon · 2024-10-28T19:01:44Z

src/end-to-end/README.md

+## CI runs
+
+The CI runs for the end-to-end tests are defined in the `.github` directory.
+
+The `Action` in Github is called `End to end testing`.
+
+- Workflow
+  - The workflow run by CI is called `workflows/e2d-testing.yml`
+  - It requires a Pinecone API key and a Vercel token.
+    - The Pinecone API key it uses the associated with the `ops@` account and is stored in a Github secret
+    - The Vercel token is from the Pinecone Enterprise account and is stored in a Github secret
+- Action
+  - The action run by the workflow is called `actions/e2e-testing/edge/action.yml`
+  - This action packages the `ts-client` code on the current branch into the external app's repo
+  - It then installs the Vercel CLI and sends a POST request to the test app's `/api/createSeedQuery` endpoint,
+    which is up and running on Vercel
+  - If the response from the endpoint does not include a match (a match to a query composed by
+    the test app), the action fails
+  - The action then deletes the index that the test app spun up (note: it will attempt to delete the index even if
+    the assertions fail)


This content doesn't seem that helpful to me. Since a lot of it is a narrative telling of the workflow, it seems likely to quickly go stale as CI configurations evolve. I would probably delete most of this, but add some explanatory comments in the workflow/job/action configs. Keeping that sort of documentation right next to the implementation will give us a better shot at keeping it up to date.

What I think would be useful here are:

Some conceptual introduction to what these tests are and why we need them, how they differ from other integration tests, etc.

Specific instructions for how to run locally. What environment variables are needed and how do I set them?

Links to the workflow source and github UI for the workflow.

jhamon · 2024-10-28T19:07:15Z

README.md

+## Testing
+
+All unit, integration, and end-to-end testing takes place automatically in CI and is configured using Github actions
+and workflows, located in the `.github` directory of this repo.
+
+To run tests locally, see [CONTRIBUTING.md](CONTRIBUTING.md) for instructions.


I think this can probably be cut. Maybe leave a note directing people to CONTRIBUTING.md for the development readme.

End-users trying to get their app built (the main audience for this readme) shouldn't ever need to run our tests. Also, explaining about the .github directory doesn't seem like the right level of granularity. Github Actions is in such widespread use on github that I think it feels like a waste of the reader's time to explain here. It's kind of like how we don't write a lot to explain other conventional aspects, like how we track module dependencies in our package.json file.

jhamon · 2024-10-28T19:24:24Z

.github/actions/e2e-testing/edge/action.yml

+    - name: Send POST request to endpoint Vercel app endpoint api/createSeedQuery
+      run: |
+        npm install -g typescript
+        npm install -g ts-node
+
+        API_KEY="${{ inputs.PINECONE_API_KEY }}"
+
+        indexName=$(ts-node src/end-to-end/localRuns.ts $API_KEY)
+
+        if [[ "$indexName" != *"edge-test"* ]]; then
+          echo "Error encountered: $indexName"
+          exit 1
+        else
+          echo "Test succeeded! Index created: $indexName"
+        fi
+
+        echo "indexName=$indexName" >> $GITHUB_ENV


This seems fine for now, but it would be cool to eventually have some sort of reusable action that let you do something like this:

- name: Call Vercel test endpoints uses: ./actions/post-vercel-endpoint with: app_url: https://ts-client-e2e-tests.vercel.app vercel_token: '${{ secrets.vercel_token }}' endpoints: - api/createSeedQuery - api/test2 - api/test3

In particular it seems like the ability to parameterize on app_url would be useful, since right now we can only deploy and test one branch at a time.

…ve re-checkout of repo

austin-denoble

LGTM, thanks for taking on all the feedback and iterating on this. I'm really excited to have this test harness in place finally! 🚢

…ncy (#311) ## Problem This is a fast-follow from [the PR that introduced the external app testing for Edge runtimes](#304). We just need to rename the GH workflow to match the rest of the files/code in the client, which we couldn't do previously w/o creating a new workflow and merging it to `main`. ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here)

Try #1, new e2e workflow

Verified

This commit was signed with the committer’s verified signature.

keith Keith Smiley

GPG key ID: 33BA60D44C7167F8

Verified
Learn about vigilant mode

Loading
Loading status checks…

8b10b2c

aulorbe commented Oct 16, 2024

View reviewed changes

.github/actions/e2e-testing/edge/action.yml Outdated Show resolved Hide resolved

aulorbe requested a review from austin-denoble October 16, 2024 21:53

aulorbe added 7 commits October 16, 2024 14:56

Remove mkdir command

Loading
Loading status checks…

bf020b2

#3

Loading
Loading status checks…

36a0799

Try to clean up indexes afterwards

Loading
Loading status checks…

6f83553

Add deletion step

Loading
Loading status checks…

ac6aedd

Change success code in deletion

Loading
Loading status checks…

378ce62

Change branch of app to main

Loading
Loading status checks…

a1ab1f8

lint, format

Loading
Loading status checks…

70d8612

aulorbe removed the request for review from austin-denoble October 16, 2024 23:05

aulorbe changed the title ~~Merge new workflow to main in order to test~~ [Draft] Oct 16, 2024

aulorbe added 3 commits October 17, 2024 15:27

Finalize details

Loading
Loading status checks…

1adf0a9

Oops, add checkout step in testing.yml

Loading
Loading status checks…

c6f1c18

Missing dash before uses

Loading
Loading status checks…

7966e6d

aulorbe changed the title ~~[Draft]~~ [Draft] e2e Oct 17, 2024

One more

Loading
Loading status checks…

49938dd

aulorbe commented Oct 18, 2024

View reviewed changes

Final edits

a3c8f7a

aulorbe changed the title ~~[Draft] e2e~~ Add end to end test suite to test NextJS app in Edge runtime w/TS client Oct 18, 2024

aulorbe requested review from austin-denoble and jhamon October 18, 2024 19:55

aulorbe commented Oct 18, 2024

View reviewed changes

aulorbe added 3 commits October 18, 2024 13:07

Remove unnecessary parts of deletion process

Loading
Loading status checks…

4d168a0

Add back in exit 1 in deletion part

Loading
Loading status checks…

d2841b5

Add part about repl to contributing doc

Loading
Loading status checks…

75006b8

austin-denoble reviewed Oct 28, 2024

View reviewed changes

aulorbe added 4 commits October 28, 2024 10:48

Encapsulate test logic into ts script instead of in GH action file

Loading
Loading status checks…

2b67942

Try global install of ts-node; do not cd into e2e dir;

Loading
Loading status checks…

268c000

Oops forgot to actually print output of localRuns to console

Loading
Loading status checks…

fa884f3

Remove clone step + update documentation

Loading
Loading status checks…

90ce139

jhamon approved these changes Oct 28, 2024

View reviewed changes

aulorbe added 13 commits October 28, 2024 15:43

Enable full run, including deletion of test index, locally

Loading
Loading status checks…

8688ea3

Rename files, get local run all the way

Loading
Loading status checks…

f60e3df

Test out new configuration for CI runs

Loading
Loading status checks…

5cc5786

Update naming

Loading
Loading status checks…

bbe48f8

Revert name change so can run workflow w/o it being on main branch

Loading
Loading status checks…

02e570b

Add Vercel token and pulldown of project vars into deploy command

Loading
Loading status checks…

eb81aff

Change names again, add --token flag to authenticate w/Vercel CLI

Loading
Loading status checks…

b3051b1

Oops, was updating wrong file

Loading
Loading status checks…

4d8a60c

trying to debug where it is actually erroring out

Loading
Loading status checks…

73ec7a2

Trying to figure out why it is failing due to ts-node not being in env

Loading
Loading status checks…

ea65803

Rename file, add --token flag again

Loading
Loading status checks…

2079ef7

Add -g flag to typescript + ts-node installs + add --prod flag + remo…

Loading
Loading status checks…

21e0c9e

…ve re-checkout of repo

Finalize URLs, paths, etc.

Loading
Loading status checks…

3241a3a

aulorbe changed the title ~~Add end to end test suite to test NextJS app in Edge runtime w/TS client~~ Add integration testing w/external NextJS app (Edge runtime) Oct 29, 2024

aulorbe added 4 commits October 29, 2024 11:07

Incorporate final feedback

Loading
Loading status checks…

f7e6551

Ooops, fix typos

Loading
Loading status checks…

5f1af7b

Rename file to make it clear it is just for local runs

Loading
Loading status checks…

f4d1959

Rename file

Loading
Loading status checks…

df866ad

austin-denoble approved these changes Oct 29, 2024

View reviewed changes

Merge branch 'main' into Audrey/e2e

Loading
Loading status checks…

ac24bc9

aulorbe merged commit d31d6da into main Oct 29, 2024
32 checks passed

aulorbe deleted the Audrey/e2e branch October 29, 2024 20:38

aulorbe mentioned this pull request Oct 31, 2024

Rename previously-named e2e workflow to external-app for consistency #311

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration testing w/external NextJS app (Edge runtime) #304

Add integration testing w/external NextJS app (Edge runtime) #304

aulorbe commented Oct 16, 2024 •

edited

Loading

aulorbe Oct 18, 2024

aulorbe Oct 18, 2024

aulorbe Oct 18, 2024

austin-denoble Oct 28, 2024

aulorbe Oct 28, 2024

aulorbe Oct 18, 2024 •

edited

Loading

aulorbe Oct 18, 2024

austin-denoble Oct 28, 2024

aulorbe Oct 28, 2024

austin-denoble left a comment

austin-denoble Oct 28, 2024

austin-denoble Oct 28, 2024

aulorbe Oct 28, 2024

austin-denoble Oct 28, 2024

austin-denoble Oct 28, 2024

austin-denoble Oct 28, 2024

aulorbe Oct 28, 2024

aulorbe Oct 28, 2024

jhamon left a comment

jhamon Oct 28, 2024

aulorbe Oct 29, 2024

jhamon Oct 28, 2024 •

edited

Loading

jhamon Oct 28, 2024

jhamon Oct 28, 2024

jhamon Oct 28, 2024

aulorbe Oct 29, 2024

austin-denoble left a comment

		@@ -27,27 +31,68 @@ runs:
		uses: GuillaumeFalourd/clone-github-repo-action@main


		response=$(curl --silent --location --request POST "$API_URL" --header "PINECONE_API_KEY: $API_KEY")

		indexName=$(echo "$response" \| jq -r '.indexName')

Add integration testing w/external NextJS app (Edge runtime) #304

Add integration testing w/external NextJS app (Edge runtime) #304

Conversation

aulorbe commented Oct 16, 2024 • edited Loading

Problem

Solution

Overview of changes

Type of Change

Test Plan

To Dos

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

aulorbe Oct 18, 2024 • 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

austin-denoble left a 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

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

jhamon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon Oct 28, 2024 • 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

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

aulorbe commented Oct 16, 2024 •

edited

Loading

aulorbe Oct 18, 2024 •

edited

Loading

jhamon Oct 28, 2024 •

edited

Loading