Refactoring Clients usage #20

erdimaden · 2024-05-20T14:13:56Z

What changed? Why?

Removed axios-mock-adapter library and replaced existing usage with jest mock functions.
Implemented API mock function tests using toHaveBeenCalledWith and toHaveBeenCalledTimes.
Replaced model access with static usage of Coinbase.apiClients.

Qualified Impact

yuga-cb · 2024-05-20T15:21:15Z

src/coinbase/tests/utils.ts

+};
+
+export const walletsApiMock = {
+  getWallet: jest.fn().mockResolvedValue(Promise.resolve({ data: VALID_WALLET_MODEL })),


None of these are setting specific expectations on the mocks. We should be verifying that the expected arguments are being passed before we return a value (or error).

Started doing that here: https://github.com/coinbase/coinbase-sdk-nodejs/pull/21/files#diff-ac84f98e46bfd44c15fed0df3bb4e8c4865c2ba08401083898fd9dbb1bb4486eR49

Need to also assert on the request body.

We definitely need to be able to mock multiple responses for things like getAddress when we derive multiple addresses in the instantiation flow.

I have addressed the feedback. All mock outputs will now be generated dynamically. I will share more details in the PR.

yuga-cb · 2024-05-21T00:21:33Z

src/coinbase/tests/utils.ts

+export const addressesApiMock = {
+  requestFaucetFunds: jest.fn().mockResolvedValue({ data: { transaction_hash: "0xdeadbeef" } }),
+  getAddress: jest.fn().mockResolvedValue({ data: VALID_ADDRESS_BALANCE_LIST }),
+  getAddressBalance: jest.fn().mockResolvedValue({ data: { VALID_BALANCE_MODEL } }),


We are still returning hard-coded values, rather than ensuring that the returned values match what was passed in as parameters.

For example, if I the mock call is getAddressBalance(walletId, addressId, assetId), then I would expect to get back a Balance model that has the same asset ID. Right now we are always returning VALID_BALANCE_MODEL, which means we are always returning an ETH balance even if the asset ID passed is something else like USDC.

So we should not be setting "mockResolvedValue" statically here; instead, we should set them on each test as appropriate.

I think these should all just be jest.fn(), no return values

Then the expectations should be set on each test

Makes sense. I changed with jest.fn()

yuga-cb · 2024-05-21T00:49:23Z

src/coinbase/tests/address_test.ts

@@ -101,85 +40,121 @@ describe("Address", () => {
  });

  it("should return the correct list of balances", async () => {
-    axiosMock.onGet().reply(200, VALID_ADDRESS_BALANCE_LIST);
+    Coinbase.apiClients.address = {


This way of replacing the value of Coinbase.apiClients.address with the existing ...addressesApiMock, and then the one method that we want to mock, seems inelegant.

Instead, I think apiClients.address should be set to a mock object for all of the tests in this file.

We should also be using more describe and befores to ensure the expectations are being set at the correct point of the test lifecycle.

+1. We also should more closely align the usage of describe, before, and it to the Ruby SDK spec tests. The test frameworks across Node.js and Ruby are quite similar so there is an opportunity for us to have very tight alignment on the structure of our tests / precisely what tests get run.

John-peterson-coinbase · 2024-05-21T16:18:43Z

src/coinbase/tests/address_test.ts

+    Coinbase.apiClients.address!.getAddressBalance = mockFn(request => {
+      const { asset_id } = request;
+      balanceModel = {
+        amount: "1000000000000000000",
+        asset: {
+          asset_id,
+          network_id: Coinbase.networkList.BaseSepolia,
+        },
+      };
+      return { data: balanceModel };
+    });
+    Coinbase.apiClients.address!.listAddressBalances = mockFn(() => {
+      return { data: VALID_ADDRESS_BALANCE_LIST };
+    });
+    Coinbase.apiClients.address!.requestFaucetFunds = mockFn(() => {
+      return { data: { transaction_hash: transactionHash } };
+    });


why are these in the beforeAll block? It might be better for us to place these closer to the test that they are used in. It is unclear which test they are used in with this setup. We can use nested describe blocks to provide further context for the tests.

Sure, I moved this beforeAll setup under the .create section. I used beforeAll because the mocks don't need to be updated after each test.

John-peterson-coinbase · 2024-05-21T16:19:48Z

src/coinbase/tests/address_test.ts

-    } catch (error) {
-      expect(error).toBeInstanceOf(APIError);
-    }
+    const getAddressBalance = jest.fn().mockRejectedValue(new APIError(""));


Mixed usage of jest.fn() and mockFn from ./utils.ts

good catch! I'm updating.

John-peterson-coinbase · 2024-05-21T16:23:42Z

src/coinbase/tests/utils.ts

+export const mockFn = (...args) => jest.fn(...args) as any;
+export const mockReturnValue = data => jest.fn().mockResolvedValue({ data });
+export const mockReturnRejectedValue = data => jest.fn().mockRejectedValue(data);


Lets consider using the builtin jest methods directly to promote standard usage.

This added layer of indirection may not be necessary. If we prefer to keep this, lets use it consistently in all test files.

Using long jest.fn().mockResolvedValue or jest.fn(...args) statements takes up too much space in test cases, so I aimed to make them shorter. Sure, I'll leave the final decision to the team.

erdimaden · 2024-05-21T16:17:16Z

src/coinbase/tests/utils.ts

+};
+
+export const walletsApiMock = {
+  getWallet: jest.fn().mockResolvedValue(Promise.resolve({ data: VALID_WALLET_MODEL })),


I have addressed the feedback. All mock outputs will now be generated dynamically. I will share more details in the PR.

erdimaden · 2024-05-21T16:17:49Z

src/coinbase/tests/utils.ts

+export const addressesApiMock = {
+  requestFaucetFunds: jest.fn().mockResolvedValue({ data: { transaction_hash: "0xdeadbeef" } }),
+  getAddress: jest.fn().mockResolvedValue({ data: VALID_ADDRESS_BALANCE_LIST }),
+  getAddressBalance: jest.fn().mockResolvedValue({ data: { VALID_BALANCE_MODEL } }),


Makes sense. I changed with jest.fn()

erdimaden · 2024-05-21T16:30:01Z

src/coinbase/tests/address_test.ts

-    } catch (error) {
-      expect(error).toBeInstanceOf(APIError);
-    }
+    const getAddressBalance = jest.fn().mockRejectedValue(new APIError(""));


good catch! I'm updating.

erdimaden · 2024-05-21T16:31:50Z

src/coinbase/tests/utils.ts

+export const mockFn = (...args) => jest.fn(...args) as any;
+export const mockReturnValue = data => jest.fn().mockResolvedValue({ data });
+export const mockReturnRejectedValue = data => jest.fn().mockRejectedValue(data);


Using long jest.fn().mockResolvedValue or jest.fn(...args) statements takes up too much space in test cases, so I aimed to make them shorter. Sure, I'll leave the final decision to the team.

erdimaden · 2024-05-21T16:32:45Z

src/coinbase/tests/wallet_test.ts

+    walletId = randomUUID();
+    // Mock the API calls
+    Coinbase.apiClients.wallet = walletsApiMock;
+    Coinbase.apiClients.address = addressesApiMock;
+    Coinbase.apiClients.wallet!.createWallet = mockFn(request => {
+      const { network_id } = request.wallet;
+      apiResponses[walletId] = {
+        id: walletId,
+        network_id,
+        default_address: newAddressModel(walletId),
+      };
+      return { data: apiResponses[walletId] };
+    });
+    Coinbase.apiClients.wallet!.getWallet = mockFn(walletId => {
+      walletModel = apiResponses[walletId];
+      return { data: apiResponses[walletId] };
+    });
+    Coinbase.apiClients.address!.createAddress = mockFn(walletId => {
+      return { data: apiResponses[walletId].default_address };
+    });


We use dynamic mocks to store wallet data in the apiResponses variable. This means that when a request is made to getWallet or createAddress, it only accesses the data for the specific walletId that was generated.

erdimaden · 2024-05-21T16:35:31Z

src/coinbase/tests/coinbase_test.ts

@@ -41,47 +44,84 @@ describe("Coinbase tests", () => {
  });

  describe("should able to interact with the API", () => {


As @yuga-cb mentioned, we will be using more describe blocks in this file. I will create a follow-up PR to implement these changes.

erdimaden · 2024-05-21T16:39:03Z

src/coinbase/tests/wallet_test.ts

-          )
-          .replyOnce(200, address);
+      jest.clearAllMocks();
+      const getAddress = jest.fn();


Here, we mock the same function twice so I don't use utils function

…te section

erdimaden · 2024-05-21T17:02:35Z

src/coinbase/coinbase.ts

@@ -42,7 +42,7 @@ export class Coinbase {
    Weth: "weth",
  };

-  apiClients: ApiClients = {};
+  static apiClients: ApiClients = {};


The Coinbase.apiClients is now globally accessible, so there's no need to pass clients to the Model class anymore.

erdimaden · 2024-05-21T17:06:07Z

src/coinbase/tests/address_test.ts

+    expect(getAddressBalance).toHaveBeenCalledWith(address.getWalletId(), address.getId(), assetId);
+    expect(getAddressBalance).toHaveBeenCalledTimes(1);


To create tests similar to the ones in Ruby, I've added toHaveBeenCalledWith and toHaveBeenCalledTimes assertions.

Reference: https://github.com/coinbase/coinbase-sdk-ruby/blob/master/spec/unit/coinbase/address_spec.rb#L94

erdimaden · 2024-05-21T17:14:30Z

I've addressed the feedback. I will get this PR merged first to unblock @John-peterson-coinbase, and then I will create a follow-up PR for the describe changes.

Remove axios-mock-adapter and update client access in Models

80cce7c

yuga-cb reviewed May 20, 2024

View reviewed changes

erdimaden added 5 commits May 20, 2024 13:47

Updating Address tests

65880bb

Updating Coinbase Class tests

59a9e35

Merge branch 'master' into feat/apiClients-refactoring

c35aea4

Merge branch 'master' into feat/apiClients-refactoring

5dee198

Updating Wallet model usage

ce39bc7

erdimaden force-pushed the feat/apiClients-refactoring branch from ab3bef5 to ce39bc7 Compare May 20, 2024 21:14

yuga-cb reviewed May 21, 2024

View reviewed changes

Updating mock usages

a507f49

erdimaden force-pushed the feat/apiClients-refactoring branch from a4e4896 to a507f49 Compare May 21, 2024 06:10

erdimaden added 2 commits May 21, 2024 10:15

updating public key

062196d

updating createWallet and getWallet mocks

1f97aff

erdimaden requested review from yuga-cb, John-peterson-coinbase and alex-stone May 21, 2024 16:00

Adding missing walletId param

e07612e

John-peterson-coinbase reviewed May 21, 2024

View reviewed changes

erdimaden commented May 21, 2024

View reviewed changes

Updating utils usage in address test and moving beforeAll under .crea…

8933f36

…te section

erdimaden commented May 21, 2024

View reviewed changes

erdimaden requested a review from John-peterson-coinbase May 21, 2024 17:14

yuga-cb approved these changes May 21, 2024

View reviewed changes

erdimaden merged commit 5ab03a9 into master May 21, 2024
6 checks passed

erdimaden deleted the feat/apiClients-refactoring branch June 21, 2024 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring Clients usage #20

Refactoring Clients usage #20

erdimaden commented May 20, 2024 •

edited

Loading

yuga-cb May 20, 2024

alex-stone May 20, 2024 •

edited

Loading

erdimaden May 21, 2024

yuga-cb May 21, 2024

yuga-cb May 21, 2024

erdimaden May 21, 2024

yuga-cb May 21, 2024

John-peterson-coinbase May 21, 2024 •

edited

Loading

John-peterson-coinbase May 21, 2024

erdimaden May 21, 2024

John-peterson-coinbase May 21, 2024

erdimaden May 21, 2024

John-peterson-coinbase May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden May 21, 2024

erdimaden commented May 21, 2024

		@@ -41,47 +44,84 @@ describe("Coinbase tests", () => {
		});

		describe("should able to interact with the API", () => {

		expect(getAddressBalance).toHaveBeenCalledWith(address.getWalletId(), address.getId(), assetId);
		expect(getAddressBalance).toHaveBeenCalledTimes(1);

Refactoring Clients usage #20

Refactoring Clients usage #20

Conversation

erdimaden commented May 20, 2024 • edited Loading

What changed? Why?

Qualified Impact

Choose a reason for hiding this comment

alex-stone May 20, 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

Choose a reason for hiding this comment

John-peterson-coinbase May 21, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdimaden commented May 21, 2024

erdimaden commented May 20, 2024 •

edited

Loading

alex-stone May 20, 2024 •

edited

Loading

John-peterson-coinbase May 21, 2024 •

edited

Loading