feat: add extrinsic integration test #211

kratico · 2022-09-07T19:02:19Z

Description

Add a simple extrinsic integration test that depends on the polkadot binary.

The polkadot binary is copied from the polkadot docker image.

How Has This Been Tested?

validate that the test workflow action has run the effect/extrinsic.test.ts test
check the test workflow logs to validate that the polkadot node was started during the test

kratico · 2022-09-07T19:04:52Z

effect/extrinsic.test.ts

+
+    await ctx.step({
+      name: "account balance",
+      ignore: true,


I've disabled this step because I'm getting the following error

account balance ... FAILED (5ms) error: InvalidStateError: readyState not OPEN ws.send(JSON.stringify(egressMessage)); ^ at WebSocket.send (deno:ext/websocket/01_websocket.js:288:15)

Very interesting –– seems I have some reproducing to do :)

kratico · 2022-09-07T19:07:02Z

.github/workflows/test.yml

+          docker run -d --entrypoint /bin/sh --rm -it --name tmp parity/polkadot:latest
+          docker cp tmp:/usr/bin/polkadot /usr/local/bin


The polkadot docker image is ubuntu based so it seems to be compatible with the GH workflow runner.

Perhaps there is a better alternative to get the polkadot binary 🤔

@pepoviola any recommendation? Should we include a runner-compatible Polkadot bin in the Capi repo? Any thoughts would be greatly appreciated.

Hey @harrysolovay / @kratico, getting the binary from the latest image is a nice way to get the binary. Other way is to download the binary from the polkadot repo, as we do in the setup command in zombienet ( https://github.com/paritytech/zombienet/blob/main/src/cli.ts#L120-L141).

Looking forward, we have the task to create a zombienet action to publish to the marketplace, then you will be able to just use the action and receive a json with the network info (relaychain validators and parachains).
Thanks!

Thank you for the input @pepoviola –– will look forward to that action! :)

harrysolovay · 2022-09-08T04:08:12Z

test-util/config.ts

    // For some reason, logs come in through `stderr`
    console.log(blue(`Piping node logs:`));
+
+    const encoder = new TextDecoder();


The initialization cost of TextDecoders is quite low. Imo not worth the overhead of a const definition.

kratico · 2022-09-08T15:25:12Z

.github/workflows/test.yml

+          curl -L -o /usr/local/bin/polkadot https://github.com/paritytech/polkadot/releases/download/v0.9.28/polkadot
+          chmod +x /usr/local/bin/polkadot


🚀 Thanks for the tip @pepoviola

kratico · 2022-09-12T12:07:12Z

effect/extrinsic.test.ts

@@ -0,0 +1,72 @@
+import { assertEquals, assertObjectMatch } from "../deps/std/testing/asserts.ts";


@harrysolovay Do you have a suggestion for this file location/name?

It seems to be more an integration test than a unit test.
Perhaps it would be better to create a new folder ./e2e-test/.. in the project root.

Thoughts?

kratico · 2022-09-12T15:50:33Z

test-util/config.ts

-    while (!isPortAvailable(port)) {
-      port++;
-    }
+    port = getRandomPort();


@harrysolovay added a random port so it's less likely to have the race condition in parallel tests

kratico · 2022-09-12T15:51:37Z

test-util/config.ts

+      port,
+      () => {
+        process.kill("SIGKILL");
+        process.close();


.close releases Deno’s memory associated with the process but it doesn't .kill the process

kratico · 2022-09-13T20:20:33Z

effect/sys/run.ts

@@ -65,6 +65,7 @@ export const { run } = (new class Runtime implements RunContext {
        if (val.exit) {
          const applied = () => val.exit!(resolved);
          cleanup.set(val, applied);
+          this.#cache.delete(k);


@harrysolovay perhaps this is fixed in your refactor.
This removes the RpcClient atom from the cache because it has an exit fn.

Is the following statement correct?
Atoms that have been cleaned up should no longer be cached

All of this will change considerably in the next few days. Will look forward to syncing about this with you & @jainkrati.

harrysolovay

Looking pretty good! Two thoughts:

Might be good to add some comments + related issues for anything that is temporary (such as the semi-thread-safe means of getting an open port)
The isPortAvailable also calls close without actually killing the process (thank you for pointing out this issue). Can you please sig-kill it as well?

harrysolovay · 2022-09-14T14:59:50Z

effect/extrinsic.test.ts

+    },
+  });
+
+  await U.throwIfError(await root.run());


No need to await U.throwIf...

harrysolovay · 2022-09-14T15:00:43Z

effect/extrinsic.test.ts

+            extrinsicEvents.push("inBlock");
+          } else if (event.params.result.finalized) {
+            extrinsicEvents.push("finalized");
+            stop();


Does it not implicitly stop?

I double checked it and the stop() needs to be called explicitly or await root.run() will not resolve/reject.

Is there an extrinsic event that signals that there will be no more events?

if so, sendAndWatchExtrinsic could implicitly call stop

harrysolovay · 2022-09-14T15:04:14Z

effect/extrinsic.test.ts

+import * as C from "../mod.ts";
+import * as t from "../test-util/mod.ts";
+import * as U from "../util/mod.ts";
+import { SendAndWatchExtrinsicProps } from "./mod.ts";


You can delete this line and then access SendAndWatchExtrinsicProps from the namespace below:

async function collectExtrinsicEvents( { config, palletName, methodName, args }: Pick< - SendAndWatchExtrinsicProps, + C.SendAndWatchExtrinsicProps,

harrysolovay · 2022-09-14T15:06:40Z

effect/extrinsic.test.ts

+  },
+});
+
+async function collectExtrinsicEvents(


This is an awesome utility. Mentioning in #215.

test-util/config.ts

harrysolovay · 2022-09-14T15:17:34Z

test-util/config.ts

+  return randomPort;
+}
+
+interface WaitForPortOptions {


These options don't seem to be supplied to the waitForPort call. Any reason to keep this declaration and the optional param on 107?

You are right.
These options can be re-added when needed, so I'll remove them.

Co-authored-by: Harry Solovay <[email protected]>

kratico · 2022-09-14T15:38:07Z

@harrysolovay

Might be good to add some comments + related issues for anything that is temporary (such as the semi-thread-safe means of getting an open port)

Will do

The isPortAvailable also calls close without actually killing the process (thank you for pointing out this issue). Can you please sig-kill it as well?

The .close in isPortAvailable is for closing a Deno.listener not a Deno.Process.
Searched for related issues and it seems that .close is enough to free up listener resources.

harrysolovay

LGTM –– please add any lessons / areas for improvement to #215

* feat: add extrinsic transfer test * ci: add polkadot binary to test workflow * chore: initialize text encoder insider the for loop * ci: download polkadot from github releases * revert: test-util/config.ts white spacing * feat: add polkadot process gracefull kill/close * feat: add random port to launch polkadot in tests * feat: add waitForPort to launch polkadot in tests * feat: add collectExtrinsicEvents helper * fix: runtime cache on cleanup * Apply suggestions from code review Co-authored-by: Harry Solovay <[email protected]> * Apply suggestions from code review Co-authored-by: Harry Solovay <[email protected]>

kratico added the WIP label Sep 7, 2022

kratico requested a review from harrysolovay as a code owner September 7, 2022 19:02

kratico commented Sep 7, 2022

View reviewed changes

harrysolovay reviewed Sep 8, 2022

View reviewed changes

kratico removed the WIP label Sep 8, 2022

kratico marked this pull request as draft September 8, 2022 11:47

kratico force-pushed the feat/extrinsic-integration-test branch from 596dbe1 to 3bfbc5b Compare September 8, 2022 14:42

kratico commented Sep 8, 2022

View reviewed changes

kratico marked this pull request as ready for review September 12, 2022 12:05

kratico commented Sep 12, 2022

View reviewed changes

kratico force-pushed the feat/extrinsic-integration-test branch from 3ac7195 to 70413e7 Compare September 12, 2022 15:48

kratico commented Sep 12, 2022

View reviewed changes

kratico added 9 commits September 12, 2022 13:12

feat: add extrinsic transfer test

24c8440

ci: add polkadot binary to test workflow

e9184ca

chore: initialize text encoder insider the for loop

96b0113

ci: download polkadot from github releases

9508018

revert: test-util/config.ts white spacing

67af348

feat: add polkadot process gracefull kill/close

2db8c54

feat: add random port to launch polkadot in tests

9fed054

feat: add waitForPort to launch polkadot in tests

8067d88

feat: add collectExtrinsicEvents helper

f35116f

kratico force-pushed the feat/extrinsic-integration-test branch from 70413e7 to f35116f Compare September 13, 2022 19:05

kratico commented Sep 13, 2022

View reviewed changes

kratico force-pushed the feat/extrinsic-integration-test branch 2 times, most recently from d1ab1ce to 8a5d306 Compare September 13, 2022 20:48

fix: runtime cache on cleanup

c471549

kratico force-pushed the feat/extrinsic-integration-test branch from 8a5d306 to c471549 Compare September 13, 2022 20:50

harrysolovay mentioned this pull request Sep 14, 2022

Expand Test Utils + Zombienet #215

Closed

harrysolovay suggested changes Sep 14, 2022

View reviewed changes

Apply suggestions from code review

555afad

Co-authored-by: Harry Solovay <[email protected]>

Apply suggestions from code review

7f25d88

kratico requested a review from harrysolovay September 14, 2022 16:12

harrysolovay approved these changes Sep 16, 2022

View reviewed changes

kratico merged commit 0b29d5c into main Sep 16, 2022

kratico deleted the feat/extrinsic-integration-test branch September 16, 2022 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add extrinsic integration test #211

feat: add extrinsic integration test #211

kratico commented Sep 7, 2022 •

edited

Loading

kratico Sep 7, 2022

harrysolovay Sep 8, 2022

kratico Sep 7, 2022

harrysolovay Sep 8, 2022

pepoviola Sep 8, 2022

harrysolovay Sep 8, 2022

harrysolovay Sep 8, 2022

kratico Sep 8, 2022

kratico Sep 12, 2022

kratico Sep 12, 2022

kratico Sep 12, 2022

kratico Sep 13, 2022

harrysolovay Sep 14, 2022

harrysolovay left a comment

harrysolovay Sep 14, 2022

harrysolovay Sep 14, 2022

kratico Sep 14, 2022 •

edited

Loading

harrysolovay Sep 14, 2022

harrysolovay Sep 14, 2022

harrysolovay Sep 14, 2022

kratico Sep 14, 2022

kratico commented Sep 14, 2022 •

edited

Loading

harrysolovay left a comment

		docker run -d --entrypoint /bin/sh --rm -it --name tmp parity/polkadot:latest
		docker cp tmp:/usr/bin/polkadot /usr/local/bin

		curl -L -o /usr/local/bin/polkadot https://github.com/paritytech/polkadot/releases/download/v0.9.28/polkadot
		chmod +x /usr/local/bin/polkadot

		@@ -0,0 +1,72 @@
		import { assertEquals, assertObjectMatch } from "../deps/std/testing/asserts.ts";

feat: add extrinsic integration test #211

feat: add extrinsic integration test #211

Conversation

kratico commented Sep 7, 2022 • 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

harrysolovay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratico Sep 14, 2022 • 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

kratico commented Sep 14, 2022 • edited Loading

harrysolovay left a comment

Choose a reason for hiding this comment

kratico commented Sep 7, 2022 •

edited

Loading

kratico Sep 14, 2022 •

edited

Loading

kratico commented Sep 14, 2022 •

edited

Loading