Add E2E tests for PSP #55

vponline · 2022-04-06T08:45:06Z

This adds E2E tests for PSP beacon updates.

Merge branch process-refactor

Resolve conflicts

Merge branch process-refactor

package.json

src/test/e2e/psp.feature.ts

vponline · 2022-04-06T13:08:38Z

Do we need tests for multiple subscriptions? I might need help with setting those up.

amarthadan

I quite like this 👍

Could we maybe add some test(s) for unsuccessful updates? What are the cases when the update can fail?

Also, since we have that retry mechanism for API calls, can we test that too?

package.json

src/test/e2e/psp.feature.ts

amarthadan · 2022-04-07T10:16:17Z

Do we need tests for multiple subscriptions? I might need help with setting those up.

I guess that would be nice. What's the problem with that?

amarthadan · 2022-04-07T10:17:04Z

@acenolaza @Siegrift Can you take a look at this as well?

amarthadan · 2022-04-07T11:05:38Z

Oh, one thing I forgot, can you update README with instructions on how to run the e2e tests?

acenolaza

I was trying my best to avoid having to add hardhat as a dep but I don't see any other way around when it comes to e2e. Unless we create a different repo that deploys an Airkeeper using the docker image etc. but that might be too much work at the moment.

The good thing is that now we could reference the contracts from lets say operations repo and build them locally to get the ABI instead of having them hardcoded in different files.

acenolaza · 2022-04-07T13:34:56Z

src/test/e2e/psp.feature.ts

+    expect(res).toEqual({
+      statusCode: 200,
+      body: JSON.stringify({ ok: true, data: { message: 'PSP beacon update execution has finished' } }),
+    });


I'm not sure that this is enough to determine if the beacon was successfully updated. I think we should be spying on the DapiServer functions in order to check that the expected ones where actually called with the expected args. WDYT?

Can we instead of spying just read the value and see if it's updated? (just asking, not sure if it would work)

Yes agreed, great suggestion, I'll add 👍

src/test/mock-utils.ts

…server values

vponline · 2022-04-08T13:02:31Z

src/test/e2e/psp.feature.ts

+    //Reset the local hardhat network state for each test to keep the deployed Airnode and DapiServer contract addresses
+    //the same as the config files
+    await hre.network.provider.send('hardhat_reset');


This took me way too long to figure this out, but there were a couple issues with the original setup:

The (dapiServer, airnodeProtocol) contracts were re-deployed for each test and since the hardhat network was running, those had different addresses for each test. Then the DapiServer and AirnodeRrp addressed in the config files didn't match and the values were not updated (even though the handler finishes correctly).

I had to make a few modifications to get this hardhat_reset to work (namely copy the hardhat.config.ts to the root after install), otherwise I got an error that he project 'was not a hardhat project'.

mockReadFileSync does not seem to be working for this (even when I change it to mockImplementation), but it errors out saying that the airnode config cannot be found. So I changed the loadConfig functions to be mocked instead.

I tried to derive the beaconid with the airnodeProtocol address and the templateId, but it ends up giving me a different id than getting it from the dapiServer contract with subscriptionIdToBeaconId(). I'm not sure why that is.

I am not sure about the 3), that seems like a thing which might bite us in production. Can you maybe check why is that? (not necessarily in this PR)

Nit: Can you add spaces to comment?

@vponline were you able to figure 3 out?

@Siegrift @acenolaza This was my bad, I was trying to derive it with the airnode protocol address instead of the airnode address from the config... 🤦 I updated beaconId to be derived now.

vponline · 2022-04-08T14:03:06Z

package.json

@@ -14,11 +14,18 @@
    "*.js"
  ],
  "scripts": {
-    "postinstall": "git clone https://github.com/api3dao/airnode.git airnode; cd airnode && git checkout 994d895f37ca871f57224ac1603ea58e79ef071e && yarn run bootstrap && yarn build && cd ..; cp -a airnode/node_modules/@api3/airnode-node/. node_modules/@api3/airnode-node; rm -rf airnode",
+    "postinstall": "git clone https://github.com/api3dao/airnode.git airnode; cd airnode && git checkout 994d895f37ca871f57224ac1603ea58e79ef071e && yarn run bootstrap && yarn build && cd ..; cp -a airnode/node_modules/@api3/airnode-node/. node_modules/@api3/airnode-node; cp -a src/test/hardhat.config.ts ./; rm -rf airnode",


We need the hardhat file in the root folder so I'm copying there after installing, this can be also removed once we don't need the postinstall script.

Merge branch main

vponline · 2022-04-12T07:46:02Z

I've added tests for retries, invalid subscriptions, a second valid subscription, updated the README, checking updated values, and fixed a problem with hardhat. I think all PR improvements are now covered so please feel free to review again 😄

amarthadan

See the last comments but otherwise LGTM 👍

ecosystem.config.js

amarthadan · 2022-04-12T08:15:46Z

src/test/config/config.ts

+  threshold: 10,
+});
+
+export const buildLocalConfig2 = () => ({


The only difference is the ETH -> BTC right? Wouldn't it be better to just override that one parameter in the test instead of the whole new quite big object?

On second thought (while reading the test code) I'm ok with it. Maybe just add a comment saying that one is an ETH and one is a BTC subscription.

hmm I thought all the subscriptionIds, templateIds, encoded parameters etc. were required to be valid? Please correct me if this is not the case for the future

They do afaik. Why do you ask? I just don't understand what it has to do with what I wrote.

Oh right, you're talking about buildLocalConfig2, for some reason I was thinking about how that would work with the airkeeper config 🤦

I'll add the comments

amarthadan · 2022-04-12T08:30:32Z

src/test/e2e/psp.feature.ts

+    });
+  });
+
+  it('updates the beacon successfully with one invalid subscription present', async () => {


I see, you're adding a new invalid subscription. It took me a while. I thought that you were failing one of those two already there. Wouldn't that be better? That way we can check that the value was actually not updated.

That's a great idea, ill update

Siegrift

👍 LGTM

Siegrift · 2022-04-12T08:56:29Z

.prettierignore

 **/.serverless/**
 **/coverage/**

 **/config/*.json
-**/node_modules/**


We also want to add airnode here temprarily.

Siegrift · 2022-04-12T08:56:55Z

README.md

+
+### Unit Tests
+
+Unit tests can then be run with:


🙏 Thanks

Siegrift · 2022-04-12T10:08:35Z

src/test/e2e/psp.feature.ts

+    //Reset the local hardhat network state for each test to keep the deployed Airnode and DapiServer contract addresses
+    //the same as the config files
+    await hre.network.provider.send('hardhat_reset');


I am not sure about the 3), that seems like a thing which might bite us in production. Can you maybe check why is that? (not necessarily in this PR)

Siegrift · 2022-04-12T10:09:31Z

src/test/e2e/psp.feature.ts

+      value: hre.ethers.utils.parseEther('1'),
+    });
+
+    // Setup ETH Subscription


Mhh, does this comment refer to something?

Yes so I added this for it to be clear that the following lines are for setting up the ETH subscription (i.e. from buildLocalConfig)

Maybe these should be renamed to buildLocalConfigETH and buildLocalConfigBTC to be clear

Siegrift · 2022-04-12T10:10:46Z

src/test/e2e/psp.feature.ts

+  it('updates the beacon successfully with one invalid provider present', async () => {
+    jest
+      .spyOn(config, 'loadAirnodeConfig')
+      .mockImplementation(


Mhh, does this work in combination with mockImplementationOnce below? (are they different actually?)

To answer your question, it does work, so mockImplementation is setting the default value to be returned after any mockImplementationOnce cases are handled

But in this case it's not needed since we want the airnode config to always include the invalid provider, I'll change this to just have mockImplementation 👍

Siegrift · 2022-04-12T10:12:12Z

src/test/e2e/psp.feature.ts

+    //Reset the local hardhat network state for each test to keep the deployed Airnode and DapiServer contract addresses
+    //the same as the config files
+    await hre.network.provider.send('hardhat_reset');


Nit: Can you add spaces to comment?

src/test/e2e/psp.feature.ts

Siegrift

One thing, why is the test folder inside the src? It should be alongside src.

amarthadan · 2022-04-14T06:11:42Z

Is this still waiting for something?

vponline · 2022-04-14T07:16:29Z

Is this still waiting for something?

Only @acenolaza's final approval 😄

acenolaza

Apologies @vponline. It's now approved 👍🏻

vponline added 9 commits April 5, 2022 12:01

Setup psp e2e test

ef7d92d

Merge branch 'process-refactor' into psp-e2e-test

ff0b9a0

Merge branch process-refactor

Setup build step

72547d5

Silence logger for e2e, set hardhat defaults

ef0184d

Fix hardhat config

af431cb

Merge branch 'process-refactor' into psp-e2e-test

ca206ac

Resolve conflicts

Add fs mock, invalid test cases

0a67aff

Merge branch 'process-refactor' into psp-e2e-test

1a7a863

Merge branch process-refactor

Add subscription steps

2d06339

vponline self-assigned this Apr 6, 2022

vponline commented Apr 6, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

Mock env, fix config mocks

82feb71

vponline commented Apr 6, 2022

View reviewed changes

src/test/e2e/psp.feature.ts Outdated Show resolved Hide resolved

vponline requested review from amarthadan and acenolaza April 6, 2022 09:19

vponline added 3 commits April 6, 2022 12:26

Set timeout

ba1675c

Remove beforeEach

19f3bcf

Move jest timeout, fix airnodeConfig mock

9e43c68

vponline commented Apr 6, 2022

View reviewed changes

src/test/e2e/psp.feature.ts Outdated Show resolved Hide resolved

Update hardhat version - fixes missing ethereum node logs

d18be5e

amarthadan reviewed Apr 7, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/test/e2e/psp.feature.ts Outdated Show resolved Hide resolved

amarthadan requested a review from Siegrift April 7, 2022 10:16

acenolaza suggested changes Apr 7, 2022

View reviewed changes

Siegrift reviewed Apr 7, 2022

View reviewed changes

src/test/mock-utils.ts Outdated Show resolved Hide resolved

Apply pr suggestions, reset hardhat network for each test, check dapi…

cfa23d1

…server values

vponline commented Apr 8, 2022

View reviewed changes

vponline added 4 commits April 8, 2022 17:10

Update README with test instructions

bec52ec

Add api call retry test case

dcabcd5

Merge branch 'main' into psp-e2e-test

236c429

Merge branch main

Add second valid subscription

1a297da

vponline requested review from acenolaza, amarthadan and Siegrift April 12, 2022 07:46

amarthadan approved these changes Apr 12, 2022

View reviewed changes

vponline added 2 commits April 12, 2022 12:47

Use real subscription for invalid test, remove unnecessary pm2 settings

d7b4ee5

Add comment about subscriptions to localConfigs

e9ea03c

Siegrift approved these changes Apr 12, 2022

View reviewed changes

Siegrift reviewed Apr 12, 2022

View reviewed changes

vponline added 5 commits April 12, 2022 14:23

Refactor, move test folder

41ced4c

Add spaces to comments

4e6665f

Fix hardhat file .gitignore

edc9fab

Derive beaconId instead of reading from contract

bfb6380

Fix tsconfig

fa75a2e

acenolaza approved these changes Apr 14, 2022

View reviewed changes

vponline merged commit 9ff35e4 into main Apr 14, 2022

vponline deleted the psp-e2e-test branch April 14, 2022 13:55

Add E2E tests for PSP #55

Add E2E tests for PSP #55

Conversation

vponline commented Apr 6, 2022

vponline commented Apr 6, 2022

amarthadan left a comment

Choose a reason for hiding this comment

amarthadan commented Apr 7, 2022

amarthadan commented Apr 7, 2022

amarthadan commented Apr 7, 2022

acenolaza 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

vponline Apr 8, 2022 • edited

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

vponline commented Apr 12, 2022

amarthadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vponline Apr 12, 2022 • edited

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

Siegrift 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

vponline Apr 12, 2022 • edited

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

Siegrift left a comment

Choose a reason for hiding this comment

amarthadan commented Apr 14, 2022

vponline commented Apr 14, 2022

acenolaza left a comment

Choose a reason for hiding this comment

vponline Apr 8, 2022 •

edited

vponline Apr 12, 2022 •

edited

vponline Apr 12, 2022 •

edited