Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integration tests #2380

Merged
merged 68 commits into from
Mar 19, 2024
Merged

feat: integration tests #2380

merged 68 commits into from
Mar 19, 2024

Conversation

BlairCurrey
Copy link
Contributor

@BlairCurrey BlairCurrey commented Jan 30, 2024

Changes proposed in this pull request

  • extracts common MASE functionality (AccountProvider, setupFromSeed, etc.) into new mock-account-servicing-lib
  • new test package in rafiki/test/integration containing test environment configuration and integration test akin to Bruno Open Payments and P2P flows.
  • new ./test/integration/scripts/run-tests.sh command to launch test environment and run tests (via run-tests package.json script)

Context

fixes #2470
fixes #2471
fixes #2474
fixes #2475

Test Running Instructions:

Currently there are port conflicts with localenv, so you will need to spin that down first. Fixing this is captured here: #2529

To run for the first time:

  • add 127.0.0.1 host.docker.internal to /etc/hosts file
  • pnpm --filter mock-account-service-lib build
  • pnpm install
  • pnpm --filter integration run-tests -b

Then normally just run:

pnpm --filter integration run-tests -b (or without -b if you dont need to rebuild images)

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Postman collection updated

Copy link

netlify bot commented Jan 30, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 20c135f
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65f8b1dad00dc600087ac55e

@@ -2,4 +2,4 @@ packages:
- 'packages/*'
- 'localenv/mock-account-servicing-entity'
# exclude packages that are inside test directories
- '!**/test/**'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was not excluding anything from what I can tell (using pnpm ls --depth -1 -r to get packages). Removed to accommodate the test/integration workspace pkg but can rework this if it's actually doing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was probably a typo and should have been tests

- cmd to start integration environment and run tests
- seeds environment on test run
- extracts common MASE functionality into new mock-account-servicing-lib
@github-actions github-actions bot added pkg: backend Changes in the backend package. pkg: mock-ase labels Feb 2, 2024
- moves graphql url to config from seed
- also removes self section which only contained unused properties
Comment on lines -1 to -4
self:
graphqlUrl: http://happy-life-bank-backend:3001/graphql
mapHostname: 'primary-mase'
openPaymentPublishedPort: 4000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the graphqlUrl to an environment variable and got rid of self because the other properties were not used. ditto for localenv/happy-life-bank/docker-compose.yml. I could not track down the original intention of these unused properties - looks like they were never used. Just mentioning it in case someone knows something I dont.

Grant request was failing with invalid signature. This was fixed by directing hte backend to use the same private key being used to create the open payments client.
@github-actions github-actions bot added the type: source Changes business logic label Feb 20, 2024
@BlairCurrey
Copy link
Contributor Author

BlairCurrey commented Mar 15, 2024

I think this is a great start and we should just merge it. I would like to add tests, though, that check that the webhook events are fired correctly and also that the Admin APIs behave correctly.

@sabineschaller Can you elaborate a bit on "fired correctly"? For most (all?) of the webhooks in these flows we are checking that we received them and identify them by type... were you thinking about checking the data on them more specifically?

@mkurapov
Copy link
Contributor

To add to the webhook discussion, we can also check the sentAmount on outgoing payment or check the receivedAmount on the incoming one to check that the payment did in fact go through

@BlairCurrey
Copy link
Contributor Author

BlairCurrey commented Mar 16, 2024

To add to the webhook discussion, we can also check the sentAmount on outgoing payment or check the receivedAmount on the incoming one to check that the payment did in fact go through

Yeah that seems like a good one to add and I'm open to suggestions on more checks. I don't think the assertions I've added cover all the assertions we should make. Perhaps all webhooks should do a more detailed "identity" check. That is, not just expect a certain type but also make sure it's the right one (by wallet address, amount, etc. ).

I just wanted to keep these tests focused on answering the question, "do the payments flows work?" and not "do the (webhook | open payments) response meet their full specifications?" because that question is better answered by unit tests. For example, adding a new field (ie metadata on outgoing payment), or changing the type of a field, or removing a field, etc. should not fail these tests unless it actually causes the flow to fail or to complete incorrectly. I mean we can have a wider discussion on that but that's the spirit that's guiding me here.

Also, I'm not against adding assertions that are effectively covered by subsequent steps failing because it can clarify things when tests fail. I just want to make sure such assertions really are of consequence in terms of the flow failing/succeeding.

@sabineschaller
Copy link
Member

@sabineschaller Can you elaborate a bit on "fired correctly"? For most (all?) of the webhooks in these flows we are checking that we received them and identify them by type... were you thinking about checking the data on them more specifically?

Sorry, for some reason, I didn't see your webhook spy in the code. Must have been tired or distracted or both. I think the webhook tests are fine.

To add to the webhook discussion, we can also check the sentAmount on outgoing payment or check the receivedAmount on the incoming one to check that the payment did in fact go through

Is that necessary? Should we not trust our unit tests that these things have happened before the webhook event was sent?

@sabineschaller
Copy link
Member

Once we have renamed the lib (because I think mock-account-servicing-lib doesn't make as much sense as mock-account-service-lib) this is a go from my side.

@mkurapov
Copy link
Contributor

Is that necessary? Should we not trust our unit tests that these things have happened before the webhook event was sent?

I think checking the OutgoingPayment sentAmount is a good extra check to have alongside making sure the ASEs received the corresponding webhooks

Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add koa-bodyparser as a dependency in test/integration

@@ -0,0 +1,271 @@
interface Account {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, but should we lower pascal/kebab case these file names?

Copy link
Contributor Author

@BlairCurrey BlairCurrey Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No thats totally fair. Such as accountProvider or account-provider? Was accustomed to this way for classes but I see its not our pattern. Looks like we are kinda inconsistent about accountProvider vs account-provider... renamed this to accountProvider (and similar in the integration package).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are pretty inconsistent, so accountProvider is fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont really have a strong preference for whatever its worth. This is kinda more what i am used to https://github.com/airbnb/javascript#naming--filename-matches-export but see snake/kebab is recommended by google's style guide https://google.github.io/styleguide/jsguide.html#file-name. I wouldn't be against some filename cleanup.

@@ -1,169 +1,12 @@
import { gql } from '@apollo/client'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this file necessary at all now given we have packages/mock-account-service-lib/src/requesters.ts?

Copy link
Contributor Author

@BlairCurrey BlairCurrey Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it has requests not in the lib and the ones in the lib are only used internally (in setupFromSeed). Commented more on this here: https://github.com/interledger/rafiki/pull/2380/files/4cd2646220d3024331afd6b933f3ee1b52dab6d2#r1521562424

If we want, we can move all the requests to the library. Probably the entire apollo client and admin client (from the integration package) in and just expose the admin client (which could have an apollo client passed in). My initial assumption was the caller may want to form their own requests, such as different fields, or return the whole response vs. just the resource data. But none of those are necessarily blockers.

Thinking about it more, having an adminClient could make sense. It could even be useful outside the context of the tests and mock-account-servicing-entity. If we want to do that I would like to create a new task for it though. It's actually a pretty self-contained task that could be worked in parallel, if we wanted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the fact that different implementations might require different fields/requests, its not difficult to add or change these requests at all anyway. Was just trying to see how much can be taken out of the MASE.

In any case, I think the MASE would benefit from a nice overhaul/update anyway, so we shouldn't spend too much time on it until we have a proper plan for where we want to take it.

}
)

// Delay gives time for webhook to be received
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we also poll here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats a good idea. initially I was thinking we couldn't do it because the first expect will fail the test, but we can just check the underlying calls on the mock instead of doing an assertion in the poll. I changed all the waits except for the few where we specifically do want to wait a defined period of time. a6b937e 53c3f30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, we could even decrease the polling frequency (although its very much not a big deal)

Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 LGTM, tested and working for me 🚀
Good to merge, can do any follow ups later

})

this.server = this.app.listen(port, () => {
console.log(`Integration server listening on port ${port}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the next PR we should just make this a pino logger and set it to silent during tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we probably dont even need these tbh. will add it to the cleanup to address one way or another.

@BlairCurrey BlairCurrey merged commit 642e7a2 into main Mar 19, 2024
22 checks passed
@BlairCurrey BlairCurrey deleted the bc/2049/rafiki-to-rafiki-test branch March 19, 2024 00:02
@BlairCurrey BlairCurrey mentioned this pull request Mar 19, 2024
9 tasks
sabineschaller added a commit that referenced this pull request Mar 19, 2024
* feat: setup basic test env

* feat: seed integration environment

- cmd to start integration environment and run tests
- seeds environment on test run
- extracts common MASE functionality into new mock-account-servicing-lib

* refactor(localenv,integration): move graphql url to config from seed

- moves graphql url to config from seed
- also removes self section which only contained unused properties

* refactor: move testenv configuration into new dir

* refactor: cleanup env vars

* feat: add webhook server, fix env vars

* chore: comment

* feat: start grant request test

* fix: eslint errors

* fix: docker compose env vars

* feat: add --build arg

* fix: incoming-payment grant initiation request

Grant request was failing with invalid signature. This was fixed by directing hte backend to use the same private key being used to create the open payments client.

* fix: ts error

* feat: implement tests through Create Quote

* chore: upgrade op client

* feat: partially implemented grant request outgoing payment test

* feat: rework to host.docker.internal

- switches hostname to host.docker.internal to resolve url mismatch problem
- finishes grant request outgoing payment test
- cleans up some configuration and test variables

* feat: add p2p flow test

* feat: add continuation step with consent mocking

* fix: rm obsolete type cast to any and comment

* feat: add create ougoing payment test

* fix: bad pnpm-lock merge

* feat: build deps in mock ase job

* feat: get non existant wallet address test

* fix: update open payments pkg

* chore: fix lint warnings

* feat: implement continuation polling

* chore: test cleanup

* refactor: generate gql in tests instead of import from lib

* chore: rm old comment

* chore: use latest http-singature-utils to match other deps

* chore: pnpm i

* chore: use latest koa-bodyparser

* chore: rm engine strict

* chore: revert rm engine strict

- fixes netlify ci failure but ultimately not the correct fix

* chore(integration): dont ignore env, rm example env

* refactor: use docker healthcheck for running tests

* chore: move healthcheck to last started docker container

* feat: use latest open payments pkg, no body requird on continuation

* chore: pnpm i, fix broken lockfile (no apollo version)

* refactor: move webhook event enum to types

* refactor: move run integration script to test/integration

* Update packages/mock-account-servicing-lib/package.json

Co-authored-by: Sabine Schaller <[email protected]>

* refactor: simplify mock-account-servicing-entity ci step

* feat(integration): exit run-tests early if containers fail to start

* feat: use pino logger

* refactor: rename mock account servicing lib

* refactor: rename class files to camel case

* fix: import correct body parser

* refactor: poll instead of delay

* refactor: simplify call to poll condition only

* fix: update filenames

* refactor: change filename name to kebab case

---------

Co-authored-by: Sabine Schaller <[email protected]>
sabineschaller added a commit that referenced this pull request Mar 19, 2024
* feat(backend): webhook max retry

* fix: tighten filter

* Update packages/documentation/src/content/docs/integration/deployment.md

Co-authored-by: Max Kurapov <[email protected]>

* test: address review feedback

* chore: add feature requests to contributing.md (#2542)

* chore: enable graphql protection - maxDepth, blockFieldSuggestions, maxCost (#2537)

* chore: add graphql depth restriction

* feat: properly enable graphql armor

* style: remove unnecessary space

* chore: add armor to auth admin server

* fix(deps): update module github.com/aws/aws-sdk-go to v1.50.38 (#2543)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency @interledger/open-payments to v6.7.2 (#2544)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(frontend): add X-Frame-Options header (#2538)

* chore(deps): update dependency @types/lodash to ^4.17.0 (#2546)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency @types/react to ^18.2.66 (#2545)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency @interledger/open-payments to v6.8.0 (#2547)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/aws/aws-sdk-go to v1.51.0 (#2548)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency @interledger/docs-design-system to ^0.3.2 (#2549)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency axios to v1.6.8 (#2556)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency isbot to v5 (#2553)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update typescript-eslint monorepo to v7 (#2552)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency koa to ^2.15.1 (#2551)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency postcss to ^8.4.36 (#2559)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update helm release redis to v18.19.3 (#2561)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat(bruno): use polling in grant continuation (#2550)

* chore: fetch latest OP schemas

* feat(bruno): use polling for grant continuation

* docs: update

* fix: bruno tests

* chore(deps): update pnpm to v8.15.5 (#2563)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat(telemetry): LIVENET now points to new livenet NLB (#2523)

* feat(telemetry): LIVENET now points to new livenet NLB

* Update packages/backend/src/config/app.ts

Co-authored-by: Max Kurapov <[email protected]>

* Update packages/backend/src/config/app.ts

Co-authored-by: Max Kurapov <[email protected]>

---------

Co-authored-by: Max Kurapov <[email protected]>
Co-authored-by: Sabine Schaller <[email protected]>

* feat(auth): improve GNAP error responses (#2400)

* feat(auth): improve GNAP error responses

* fix: add extra arg to ctx.throw

* feat: middleware error improvements

* feat: add error messages to grant details endpoint

* fix: update error messages in grant routes

* feat: create utility function to generate GNAP errors, create enum for GNAP error codes

* feat: helper function throws instead of generates response

* feat: integration tests (#2380)

* feat: setup basic test env

* feat: seed integration environment

- cmd to start integration environment and run tests
- seeds environment on test run
- extracts common MASE functionality into new mock-account-servicing-lib

* refactor(localenv,integration): move graphql url to config from seed

- moves graphql url to config from seed
- also removes self section which only contained unused properties

* refactor: move testenv configuration into new dir

* refactor: cleanup env vars

* feat: add webhook server, fix env vars

* chore: comment

* feat: start grant request test

* fix: eslint errors

* fix: docker compose env vars

* feat: add --build arg

* fix: incoming-payment grant initiation request

Grant request was failing with invalid signature. This was fixed by directing hte backend to use the same private key being used to create the open payments client.

* fix: ts error

* feat: implement tests through Create Quote

* chore: upgrade op client

* feat: partially implemented grant request outgoing payment test

* feat: rework to host.docker.internal

- switches hostname to host.docker.internal to resolve url mismatch problem
- finishes grant request outgoing payment test
- cleans up some configuration and test variables

* feat: add p2p flow test

* feat: add continuation step with consent mocking

* fix: rm obsolete type cast to any and comment

* feat: add create ougoing payment test

* fix: bad pnpm-lock merge

* feat: build deps in mock ase job

* feat: get non existant wallet address test

* fix: update open payments pkg

* chore: fix lint warnings

* feat: implement continuation polling

* chore: test cleanup

* refactor: generate gql in tests instead of import from lib

* chore: rm old comment

* chore: use latest http-singature-utils to match other deps

* chore: pnpm i

* chore: use latest koa-bodyparser

* chore: rm engine strict

* chore: revert rm engine strict

- fixes netlify ci failure but ultimately not the correct fix

* chore(integration): dont ignore env, rm example env

* refactor: use docker healthcheck for running tests

* chore: move healthcheck to last started docker container

* feat: use latest open payments pkg, no body requird on continuation

* chore: pnpm i, fix broken lockfile (no apollo version)

* refactor: move webhook event enum to types

* refactor: move run integration script to test/integration

* Update packages/mock-account-servicing-lib/package.json

Co-authored-by: Sabine Schaller <[email protected]>

* refactor: simplify mock-account-servicing-entity ci step

* feat(integration): exit run-tests early if containers fail to start

* feat: use pino logger

* refactor: rename mock account servicing lib

* refactor: rename class files to camel case

* fix: import correct body parser

* refactor: poll instead of delay

* refactor: simplify call to poll condition only

* fix: update filenames

* refactor: change filename name to kebab case

---------

Co-authored-by: Sabine Schaller <[email protected]>

* fix(deps): update dependency isbot to ^5.1.2 (#2567)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency @types/react to ^18.2.67 (#2566)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/aws/aws-sdk-go to v1.51.2 (#2565)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update helm release redis to v18.19.4 (#2570)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat(backend): make SPSP optional (#2560)

* feat: flag to enable/disable SPSP

* test: update to cover all cases

* docs: add link to glossary

* chore: fix lockfile

* test: address feedback

* delete lockfile

* add lockfile

* delete lockfile

* fix lockfile this time?

* fix: formatting

---------

Co-authored-by: Max Kurapov <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: xplicit <[email protected]>
Co-authored-by: Nathan Lie <[email protected]>
Co-authored-by: Blair Currey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: mock-ase type: ci Changes to the CI type: source Changes business logic
Projects
None yet
3 participants