fix: rewrite sidecar e2e script #618

TarikGul · 2021-07-23T03:43:18Z

closes: #611

Rewritten e2e tests script in typescript.

scripts/config.ts

jsdw

Cool! Offhand this looks like a definite step up from the python approach (and I for one like that the python dependency goes away :D)

Does the README need any tweaks now that python is no longer a dependency and such?

scripts/runChainTests.ts

TarikGul · 2021-07-23T17:52:14Z

@jsdw Yea exactly. I need to update all the docs, and the scripts

TarikGul · 2021-07-24T05:58:25Z

@jsdw I changed it up quite a bit if you want to take a second check. There were still some node processes that would not be handled correctly on random tests, so I ended up spawning them in their own groups and using pid ranges to kill all child processes and any possible descendants. All issues are completely handled now :)

emostov · 2021-07-26T17:07:07Z

scripts/.eslintrc.js

@@ -0,0 +1 @@
+module.exports = require('@substrate/dev/config/eslint');


Is eslint smart enough to look in the directory above?

Yes! 100% is. TIL :)

removed all the eslint configs from the scripts folder.

except the eslintignore

scripts/runChainTests.ts

emostov · 2021-07-26T17:18:31Z

scripts/runChainTests.ts

+// Kill all processes spawned and tracked by this file.
+const killAll = () => {
+	console.log('Killing all processes...');
+	for (const key of Object.keys(procs)) {


I don't fully understand the considerations here, but is there any benefit in have procs as a param to this function? And generally explicitly passing procs around. Otherwise it is a global-ish object for this module, which is fine but I think might make refactoring in the future harder

is there any benefit in have procs as a param to this function

I dont think in this situation there is much benefit. Preferably the script is completely stand alone, and doesn't interact with anything else which would stop any collisions or issues happening with procs.

emostov · 2021-07-26T17:19:48Z

scripts/runChainTests.ts

+				process.kill(-procs[key].pid, 'SIGTERM');
+				process.kill(-procs[key].pid, 'SIGKILL');
+			} catch (e) {
+				/**


could console.error the message, but up to you

Yea i thought about it too, but it will throw an error every time when using -procs[key].pid here because there will be some 'pid' that sidecar already closed implicitly but is still within the group. The error is harmless, but the major thing is just making sure all process's close.

scripts/types.ts

Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>

…rate-api-sidecar into tarik-update-e2e-script

insipx

LGTM, scripts ran on the first try, looks like a good upgrade

jsdw

Looks great! I tried it and, for the first time ever, all tests passed without a hitch. I'll consider that a win :D

jsdw · 2021-07-27T08:31:07Z

README.md


-    Note that the e2e tests will connect to running nodes in order to test sidecar against real data, and they may fail owing to those connections taking too long to establish. If you run into any failures, try running tests just for the chain that failed with something like `yarn test:init-e2e-tests:polkadot`.
+    Note: that the e2e tests will connect to running nodes in order to test sidecar against real data, and they may fail owing to those connections taking too long to establish. If you run into any failures, try running tests just for the chain that failed with something like `yarn test:init-e2e-tests:polkadot`.


Nit: Maybe we don't need to repeat the per-chain bit now it's added above (but no harm in it really!)

It might be worth keeping for now, but yea I am happy to remove it if it seems like extra bloat.

jsdw · 2021-07-27T08:52:13Z

scripts/runChainTests.ts

+/**
+ * Signal interrupt
+ */
+process.on('SIGINT', function () {


I wonder whether it's worth covering a few more bases and also cleaning up when SIGKILL, SIGTERM (eg when kill command used) and SIGHUP (eg when terminal window closed) are sent?

Yea I agree, I just added SIGHUP.

cleaning up when SIGKILL, SIGTERM (eg when kill command used)

Curious what you mean by that?

Oh, I just meant that SIGTERM is the default signal sent when kill is used. Actually, I don't think you can catch SIGKILL looking at the kill man page so that's not useful anyway!

(I thnk that catching the result of ctrl+c is most important though and you've done that, and now you've added the "terminal" closed signal that seems good to me!)

Ahh okay cool i see what you are referencing at :)

TarikGul added 5 commits July 22, 2021 23:36

fix: rewrite sidecar e2e script

9db655a

fix: grumbles

fe8be29

fic: refactor

a562b51

fix: update configs

1a2fce9

fix: refactor

a04bf50

TarikGul commented Jul 23, 2021

View reviewed changes

scripts/config.ts Outdated Show resolved Hide resolved

jsdw approved these changes Jul 23, 2021

View reviewed changes

scripts/runChainTests.ts Outdated Show resolved Hide resolved

TarikGul added 2 commits July 23, 2021 13:34

fix: add CLI, fix child descendents

9f4cdaa

fix: cleanup

f67829f

TarikGul added 4 commits July 23, 2021 14:27

fix: type StatusCode => enum StatusCode

6a14cc5

fix: refactor

c3f0bfd

fix: update scripts

f00b70a

docs: update docs

c1378e5

TarikGul requested review from emostov, maciejhirsz, niklasad1, dvdplm and insipx July 23, 2021 19:20

TarikGul added 5 commits July 23, 2021 22:04

fix: comments

f2e38bf

fix: add consoles for single chain

080abd3

fix: refactor and comments

0d9b058

fix: lint

7060364

fix: handle void promise

b03db86

jsdw self-requested a review July 24, 2021 09:19

fix: typos

6b9c299

emostov reviewed Jul 26, 2021

View reviewed changes

scripts/runChainTests.ts Outdated Show resolved Hide resolved

emostov reviewed Jul 26, 2021

View reviewed changes

scripts/types.ts Outdated Show resolved Hide resolved

TarikGul and others added 3 commits July 26, 2021 14:24

Update scripts/types.ts

a8454a3

Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>

fix: refactor eslint config

22d3f4f

Merge branch 'tarik-update-e2e-script' of github.com:paritytech/subst…

2c75752

…rate-api-sidecar into tarik-update-e2e-script

insipx approved these changes Jul 26, 2021

View reviewed changes

fix: fix comments

6a7b219

jsdw approved these changes Jul 27, 2021

View reviewed changes

fix: add SIGHUP

338d237

TarikGul merged commit 423574e into master Jul 27, 2021

TarikGul deleted the tarik-update-e2e-script branch July 27, 2021 14:16

TarikGul mentioned this pull request Jul 27, 2021

chore(release): 9.0.1 #621

Closed

jsdw mentioned this pull request Jul 27, 2021

chore(release): 9.1.0 #622

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rewrite sidecar e2e script #618

fix: rewrite sidecar e2e script #618

TarikGul commented Jul 23, 2021 •

edited

Loading

jsdw left a comment

TarikGul commented Jul 23, 2021

TarikGul commented Jul 24, 2021

emostov Jul 26, 2021

TarikGul Jul 26, 2021

TarikGul Jul 26, 2021

emostov Jul 26, 2021 •

edited

Loading

TarikGul Jul 26, 2021

emostov Jul 26, 2021

TarikGul Jul 26, 2021 •

edited

Loading

insipx left a comment

jsdw left a comment

jsdw Jul 27, 2021

TarikGul Jul 27, 2021

jsdw Jul 27, 2021 •

edited

Loading

TarikGul Jul 27, 2021

jsdw Jul 27, 2021

jsdw Jul 27, 2021

TarikGul Jul 27, 2021

		@@ -0,0 +1 @@
		module.exports = require('@substrate/dev/config/eslint');


		Note that the e2e tests will connect to running nodes in order to test sidecar against real data, and they may fail owing to those connections taking too long to establish. If you run into any failures, try running tests just for the chain that failed with something like `yarn test:init-e2e-tests:polkadot`.
		Note: that the e2e tests will connect to running nodes in order to test sidecar against real data, and they may fail owing to those connections taking too long to establish. If you run into any failures, try running tests just for the chain that failed with something like `yarn test:init-e2e-tests:polkadot`.

fix: rewrite sidecar e2e script #618

fix: rewrite sidecar e2e script #618

Conversation

TarikGul commented Jul 23, 2021 • edited Loading

jsdw left a comment

Choose a reason for hiding this comment

TarikGul commented Jul 23, 2021

TarikGul commented Jul 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emostov Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TarikGul Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

insipx left a comment

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jul 27, 2021 • 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

TarikGul commented Jul 23, 2021 •

edited

Loading

emostov Jul 26, 2021 •

edited

Loading

TarikGul Jul 26, 2021 •

edited

Loading

jsdw Jul 27, 2021 •

edited

Loading