refactor: test steps into fns and make each flow 1 test #2608

BlairCurrey · 2024-03-25T17:03:27Z

Changes:

adds Peer to Peer - Cross Currency flow test
adds Open Payments with Finish Method flow test (as opposed to the continuation polling)
squashes each flow into 1 test - see the "Original PR body" collapse section below for more details.
moves each step in the tests into a admin or openPayments test action. Makes things a bit more repeatable and the test a bit higher leve - see the "Original PR body" collapse section for more details.

Context:

This is to merge into bc/2529/cleanup-integration-test, not main. I opened a separate PR in hopes of making these changes more digestable.

I originally opened this PR to get feedback on how I should refactor the tests (leave as-is, squash flows to single test but in-line everything, or squash flows to single test and turn the different steps into functions). I preserved that in the collapse section below, and it's what Max's comment is replying to here #2608 (comment):

Original PR Body

details

My main question is: how should we structure the tests? I see a couple options:

Keep them how they are in main. That is, 1 test per request in the flow and sharing data between them via global scope.
1 test per flow, with all requests/assertions in-lined and variables scoped to that test.
1 test per flow with requests/assertions abstracted into repeatable functions that can be used across flows.

So for #2 or #3, our test would output like this instead of detailing each request in the flow.:

  Integration tests
    Requests
      ✓ Can Get Non-Existing Wallet Address (337 ms)
    Flows
      ✓ Open Payments with Continuation via Polling (1 ms)
      ✓ Open Payments with Continuation via "interact_ref"
      ✓ Peer to Peer (1374 ms)
      ✓ Peer to Peer - Cross Currency (1196 ms)

I'm pursuing #3, wondering if #2 might be better, and want to shared why I don't think #1 is a good option.

These changes start to implement #3. I converted the Peer to Peer flow to 1 test and made a function for what were previously individual tests (createReceiver, createQuote, createOutgoingPayment, getOutgoingPayment). Then I implemented the Peer to Peer - Cross Currency flow test to see how reusable these functions were - I found that not too much had to be changed. Just the beginning and end of the flow basically. The input for createReceiver needs to be passed in wherever it's called, and some of the final assertions of the outgoing payments were flow specific so I pulled some assertions out to the callers. I think this is actually fine, but im wondering if the more these are re-used on different variations of the flows, the more we will have to pull out the assertions and spies and handle them where they are called. In which case this would really be over-abstracted as it would basically just wrap the gql/open payments client requests.

I'm doing this because I think option #1 is a problem basically because the different steps in the flow (create incoming payment, create quote, etc) are not independent. This means:

Flows should fail at the first failed test, which is not possible if each step is a test. With a test per flow, the first failed assertion will end the test and move to the next one (another flow). Whereas currently the next test is another step in the flow. No way around this in jest. It's just misusing the concept of tests.
We really only need to know if the flow passes/succeeds. We will still be able to see where it fails when it fails which is all that matters.
We have to thrust lots of data into the global scope so that tests can share results with subsequent test. This makes it hard to see exactly what each test relies on, and what side effects each test may have. This generally makes the tests a lot harder to understand.

I chose #3 over #2 because there would be a lot of code duplication for doing variations on flows (for example: Open Payments with Continuation via Polling and Open Payments with Continuation via "interact_ref"). However if it proves that we can't absrtact much of the underling api calls, spying/assertions then they aren't really reusable anyways I guess.

refactor changes tests to contain entire flow, instead of seperate tests for each step of flow, which are not independent.

mkurapov · 2024-03-25T22:03:10Z

@BlairCurrey I think your intuition is right, #3 seems to be the best. 1 integration test = 1 jest test makes sense as well, since it encapsulates a single flow, and each step depends on the success of the previous.

Likewise, abstracting into functions just makes individual test case easier to read as well 👍

…-integration-tests-refactor

BlairCurrey · 2024-03-27T04:07:47Z

test/integration/integration.test.ts

-      expect(payment.state).toBe(OutgoingPaymentState.Completed)
-      expect(payment.receiveAmount.value).toBe(amountValueToSend)
-      expect(payment.sentAmount.value).toBe(amountValueToSend)
+      await getOutgoingPayment(outgoingPayment.id, value)


this is in a new p2p cross currency test case .

there are some assertions in getOutgoingPayment for one of the amounts, but wasn't sure what else would be a good check. I didn't necessarily want to assume a certain conversion rate/fee structure was used, nor do I want to duplicate that calculation logic. Could just check asset code, and maybe amounts, are different?

want to assume a certain conversion rate/fee structure was used

Since we have access to the config file, would we be able to get the rates from there and confirm the amounts that way?

I think there are fees too, so we'd need to get that from the seed as well. And then duplicate the non-trivial backend logic and live with the fact that it could change, or extract that out and use it here. What do you think? Seem like not great options IMO. I suppose maybe if really want to verify those amounts I can just hardcode what they currently are and state the assumptions in a comment (fee, rates, starting amount)?

I guess im not sure, but perhaps those details aren't really that critical in this test? We're testing (or should be) those calculations and whether or not they're returned in the backend already. I think the important part of this test is that we can get the payment and see that its complete.

Maybe checking that the conversion went in the right direction would be good enough? Like if we converted between currencies with a 2:1 ratio that the amount went down as opposed to up?

that's a good idea. Added that check. e6bd5af

…-integration-tests-refactor

mkurapov

LGTM, nothing blocking, just some thoughts

mkurapov · 2024-03-28T18:28:02Z

test/integration/lib/integration-server.ts

 import Koa from 'koa'
 import bodyParser from '@koa/bodyparser'
 import http from 'http'
-import { v4 as uuid } from 'uuid'


huh I guess we don't need it anymore anywhere? that's nice.

yeah I was using it it validate some id was a uuid but is was unecessary. It was used to generate a uuid but I used crypto.randomUUID instead. I guess normally it would be good to use the same uuid package everywhere but it was just for something throwaway in the test so i figured 1 less dependency was preferable.

mkurapov · 2024-03-28T19:28:07Z

test/integration/lib/test-actions/index.ts

+    consentInteraction: (outgoingPaymentGrant, senderWalletAddress) =>
+      consentInteraction(deps, outgoingPaymentGrant, senderWalletAddress),
+    consentInteractionWithInteractRef: (
+      outgoingPaymentGrant,
+      senderWalletAddress
+    ) =>
+      consentInteractionWithInteractRef(
+        deps,
+        outgoingPaymentGrant,
+        senderWalletAddress
+      ),


maybe we can pull these out into identity provider actions (or something along those lines)

We could. Although I was thinking it would be good to let these just be miscellaneous ones so we dont have to categorize everything. The admin api and open payments api are obvious and import to differentiate (because there are duplicate createOutgoingPayment, etc.) but beyond that IDK how helpful it is to add the extra scope.

On a similar note, I originally was thinking I could do AdminTestActions, OpenPaymentTestActions, etc. instead of everything scoped to 1 TestActions. But then wondered where to throw these consent interaction ones. I guess IdentityProviderTestActions could be the answer but I didn't necessarily want to think of a scope for every potential new one.

mkurapov · 2024-03-28T19:30:53Z

test/integration/integration.test.ts

-      expect(payment.state).toBe(OutgoingPaymentState.Completed)
-      expect(payment.receiveAmount.value).toBe(amountValueToSend)
-      expect(payment.sentAmount.value).toBe(amountValueToSend)
+      await getOutgoingPayment(outgoingPayment.id, value)


want to assume a certain conversion rate/fee structure was used

Since we have access to the config file, would we be able to get the rates from there and confirm the amounts that way?

mkurapov · 2024-04-02T12:37:35Z

test/integration/integration.test.ts

+      if (exchangeRate > 1) {
+        expect(sentAmount.value).toBeGreaterThan(receiveAmount.value)
+      } else if (exchangeRate < 1) {
+        expect(sentAmount.value).toBeLessThan(receiveAmount.value)


will this always be true though? depending on the fees, you might run into a scenario where the exchange rate is 0.98 or something, but with an added fee, this will fail this assertion. I guess what this means is that:

I suppose maybe if really want to verify those amounts I can just hardcode what they currently are and state the assumptions in a comment (fee, rates, starting amount).

hardcoding the assumptions will always have to be the case.

With that said, I think we should either directly verify the amount, or not do the check at all. We can also leave it for a future enhancement, if you want. Thoughts?

Yeah I guess we would need to add some assertions checking the fee being 0. Which leads to you point about still having to hardcode assumptions (which I am also doing with the asset scales already).

I suppose making assertions (not jest expect, but assert) for fees, exchange rates, asset scale, asset code, and then expecting certain values is probably the better way to go. I'l include it here - this PR is still waiting on approval of the parent and the parents is waiting on the node 20 changes.

added 0dda0e4

feel free to give anymore feedback on it but im merging this branch into the parent.

BlairCurrey · 2024-04-02T17:15:01Z

test/integration/integration.test.ts

      const exchangeRate =
-        c9.config.seed.rates[receiverAssetCode][senderAssetCode]
-      const { sentAmount, receiveAmount } = payment
+        hlb.config.seed.rates[senderAssetCode][receiverAssetCode]
+      const fee = c9.config.seed.fees.find((fee: Fee) => fee.asset === 'USD')


did some checks to ensure these are the operative rates and fees.

* chore(integration): rm debug log * feat: build images by default * fix: testenv/localenv port conflict * fix: exit process if error in beforeAll, dont run tests * feat: use run-tests to manage test dep building * feat: integration package readme * feat: mock-account-service-lib readme * fix: use consistent filenames tyle (kebab-case) * feat(integration): add assertion confirming payment completion * feat: add cross currency flow and start refactor of tests refactor changes tests to contain entire flow, instead of seperate tests for each step of flow, which are not independent. * Revert "feat: add cross currency flow and start refactor of tests" This reverts commit 66ede8b. * fix: pnpm lock, eslint * feat: add packages to labeler * fix: apollo type mismatch * fix: renovate ignoring integration test package * feat: add get incoming payment step * fix(integration): readme type, clarify accounting is mock * Update test/integration/README.md Co-authored-by: Max Kurapov <[email protected]> * Update test/integration/scripts/run-tests.sh Co-authored-by: Max Kurapov <[email protected]> * fix(integration): typo * refactor: attempt to effectively remove ignored path from config * refactor: test steps into fns and make each flow 1 test (#2608) * feat: add cross currency flow and start refactor of tests refactor changes tests to contain entire flow, instead of seperate tests for each step of flow, which are not independent. * fix: typo * refactor: test steps into fns and make each flow 1 test * feat: add packages to labeler * fix: apollo type mismatch * fix: renovate ignoring integration test package * feat(integration): get public incoming payment test action * chore: add cross currency amount assertions * feat(mock-account-servicing-lib): export types * feat(integration): cross currency assertions * fix: ts error * chore: revert temp fix for eslint/node version mismatch - unpinned eslint to previous version --------- Co-authored-by: Max Kurapov <[email protected]>

feat: add cross currency flow and start refactor of tests

69afca4

refactor changes tests to contain entire flow, instead of seperate tests for each step of flow, which are not independent.

BlairCurrey added the do not merge Do not merge PRs with these label label Mar 25, 2024

fix: typo

51c04f3

BlairCurrey added 2 commits March 26, 2024 08:58

Merge branch 'bc/2529/cleanup-integration-tests' into bc/2529/cleanup…

08f32e3

…-integration-tests-refactor

refactor: test steps into fns and make each flow 1 test

148dfab

BlairCurrey changed the title ~~feat: add cross currency flow and start refactor of tests~~ refactor: test steps into fns and make each flow 1 test Mar 27, 2024

BlairCurrey marked this pull request as ready for review March 27, 2024 03:54

BlairCurrey requested review from sabineschaller, mkurapov and njlie March 27, 2024 03:55

BlairCurrey mentioned this pull request Mar 27, 2024

feat: misc integration test improvents #2595

Merged

5 tasks

BlairCurrey commented Mar 27, 2024

View reviewed changes

BlairCurrey added 2 commits March 27, 2024 14:53

Merge branch 'bc/2529/cleanup-integration-tests' into bc/2529/cleanup…

31f92b9

…-integration-tests-refactor

feat: add packages to labeler

d56bfe6

github-actions bot added the type: ci Changes to the CI label Mar 27, 2024

BlairCurrey added 2 commits March 27, 2024 15:37

fix: apollo type mismatch

60ff900

fix: renovate ignoring integration test package

8f5fe12

github-actions bot added the type: tests Testing related label Mar 27, 2024

BlairCurrey removed the do not merge Do not merge PRs with these label label Mar 28, 2024

Merge branch 'bc/2529/cleanup-integration-tests' into bc/2529/cleanup…

3529627

…-integration-tests-refactor

github-actions bot removed the type: ci Changes to the CI label Mar 28, 2024

feat(integration): get public incoming payment test action

964d4e2

mkurapov approved these changes Mar 28, 2024

View reviewed changes

njlie approved these changes Mar 28, 2024

View reviewed changes

chore: add cross currency amount assertions

e6bd5af

mkurapov reviewed Apr 2, 2024

View reviewed changes

BlairCurrey added 2 commits April 2, 2024 13:01

feat(mock-account-servicing-lib): export types

18203a5

feat(integration): cross currency assertions

0dda0e4

github-actions bot added the pkg: mock-account-service-lib label Apr 2, 2024

BlairCurrey merged commit 2d76382 into bc/2529/cleanup-integration-tests Apr 2, 2024
16 checks passed

BlairCurrey deleted the bc/2529/cleanup-integration-tests-refactor branch April 2, 2024 17:13

BlairCurrey commented Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: test steps into fns and make each flow 1 test #2608

refactor: test steps into fns and make each flow 1 test #2608

BlairCurrey commented Mar 25, 2024 •

edited

Loading

mkurapov commented Mar 25, 2024

BlairCurrey Mar 27, 2024 •

edited

Loading

mkurapov Mar 28, 2024

BlairCurrey Mar 28, 2024

njlie Mar 28, 2024

BlairCurrey Apr 1, 2024

mkurapov left a comment

mkurapov Mar 28, 2024

BlairCurrey Mar 28, 2024

mkurapov Mar 28, 2024

BlairCurrey Mar 28, 2024 •

edited

Loading

mkurapov Mar 28, 2024

mkurapov Apr 2, 2024

BlairCurrey Apr 2, 2024 •

edited

Loading

BlairCurrey Apr 2, 2024 •

edited

Loading

BlairCurrey Apr 2, 2024 •

edited

Loading

refactor: test steps into fns and make each flow 1 test #2608

refactor: test steps into fns and make each flow 1 test #2608

Conversation

BlairCurrey commented Mar 25, 2024 • edited Loading

Changes:

Context:

Original PR Body

mkurapov commented Mar 25, 2024

BlairCurrey Mar 27, 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

mkurapov 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

BlairCurrey Mar 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

BlairCurrey Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey commented Mar 25, 2024 •

edited

Loading

BlairCurrey Mar 27, 2024 •

edited

Loading

BlairCurrey Mar 28, 2024 •

edited

Loading

BlairCurrey Apr 2, 2024 •

edited

Loading

BlairCurrey Apr 2, 2024 •

edited

Loading

BlairCurrey Apr 2, 2024 •

edited

Loading