Feature/proposals test coverage #399

gleb-urvanov · 2020-05-07T13:59:34Z

Require Constantinople joystream types (https://github.com/Joystream/apps/tree/joystream-constantinople)

…runtime-joystream into development

mnaamani · 2020-05-19T08:37:48Z

tests/network-tests/package.json

  },
  "dependencies": {
-    "@joystream/types": "^0.7.0",
+    "@joystream/types": "../joystream-apps/packages/joy-types",


Suggested change

"@joystream/types": "../joystream-apps/packages/joy-types",

"@joystream/types": "^0.9.1",

Final constantinople release

We might as well also use alias @constantinople/types ?

…ntation

Cancellation fee: fetch from rutime

mnaamani

So overall I think the setup seems better with tap, but I left a bunch of suggestions and questions too.

I was able to run the test but I had to manually edit the chainspec.json file because the run-test-chain.sh script didn't work.

 FAIL  src/constantinople/tests/proposals/updateRuntimeTest.ts
 ✖ timeout!

  test: "  Upgrading the runtime test"
  expired: TAP

 FAIL  src/constantinople/tests/proposals/updateRuntimeTest.ts 1 failed of 12 15m
 ✖ timeout!

I couldn't really figure out why it failed.

The test in total took quite a while to run, surely with control of the elections and setting grace periods to 0 this should have been much shorter?

Suites:   1 failed, 8 passed, 9 of 9 completed
Asserts:  1 failed, 107 passed, of 108
Time:     38m

I think the helper methods for awaiting for Event to be emitted by transactions could be improved maybe to wait for a specific amount of time/blocks before throwing so maybe the test can pickup on failure of what event expectation caused the failure.

scripts/run-test-chain.sh

mnaamani · 2020-05-22T10:19:05Z

scripts/run-test-chain.sh

+#!/bin/bash
+cargo run --release -p joystream-node build-spec --chain dev > chainspec.json
+sed -i 's/"setValidatorCountProposalGracePeriod":.*/"setValidatorCountProposalGracePeriod": 0,/' chainspec.json
+sed -i 's/"runtimeUpgradeProposalGracePeriod":.*/"runtimeUpgradeProposalGracePeriod": 0,/' ./chainspec.json


Suggested change

sed -i 's/"runtimeUpgradeProposalGracePeriod":.*/"runtimeUpgradeProposalGracePeriod": 0,/' ./chainspec.json

sed -i 's/"runtimeUpgradeProposalGracePeriod":.*/"runtimeUpgradeProposalGracePeriod": 0,/' chainspec.json

mnaamani · 2020-05-22T10:19:31Z

scripts/run-test-chain.sh

+cargo run --release -p joystream-node build-spec --chain dev > chainspec.json
+sed -i 's/"setValidatorCountProposalGracePeriod":.*/"setValidatorCountProposalGracePeriod": 0,/' chainspec.json
+sed -i 's/"runtimeUpgradeProposalGracePeriod":.*/"runtimeUpgradeProposalGracePeriod": 0,/' ./chainspec.json
+sed -i 's/"setElectionParametersProposalGracePeriod":.*/"setElectionParametersProposalGracePeriod": 0,/' ./chainspec.json


Suggested change

sed -i 's/"setElectionParametersProposalGracePeriod":.*/"setElectionParametersProposalGracePeriod": 0,/' ./chainspec.json

sed -i 's/"setElectionParametersProposalGracePeriod":.*/"setElectionParametersProposalGracePeriod": 0,/' chainspec.json

mnaamani · 2020-05-22T10:24:57Z

scripts/run-test-chain.sh

@@ -0,0 +1,13 @@
+#!/bin/bash
+cargo run --release -p joystream-node build-spec --chain dev > chainspec.json


Although this approach is temporary and eventually we will have parameterized build of the runtime, and producing this chainspec file as an artifact of running tests will not be necessary, it should be generated in some temporary location folder, maybe the system temporary path?

This goes for any other temporary artifacts of course.

done, .tmp folder introduced

mnaamani · 2020-05-22T10:29:37Z

tests/network-tests/src/constantinople/tests/impl/electingCouncil.ts

+import { ApiWrapper } from '../../utils/apiWrapper';
 import { WsProvider, Keyring } from '@polkadot/api';
-import { initConfig } from './utils/config';
-import BN = require('bn.js');


I'm surprised this syntax ever worked in the first place! :)

mnaamani · 2020-05-22T12:05:30Z

tests/network-tests/src/constantinople/tests/impl/electingCouncil.ts

  const greaterStake: BN = new BN(+process.env.COUNCIL_STAKE_GREATER_AMOUNT!);
  const lesserStake: BN = new BN(+process.env.COUNCIL_STAKE_LESSER_AMOUNT!);
-  const defaultTimeout: number = 120000;
+  const defaultTimeout: number = 300000;


I don't understand why the timeout had to be increased here, and in some other tests also.

Wouldn't it be better if the timeout value for each scenario was derived based on how many finalized blocks the tests should be completed within, and the chain's block interval?

Its also not clear, is this timeout for the entire suite or per .test()? If it is per test then the timeout should be specific to each test, not the max value to accommodate all the tests.

The timeout time now is defined using number of blocks required for the test to pass.

mnaamani · 2020-05-22T12:18:34Z

tests/network-tests/src/constantinople/tests/impl/membershipCreation.ts

  initConfig();
  const keyring = new Keyring({ type: 'sr25519' });
  const N: number = +process.env.MEMBERSHIP_CREATION_N!;
  const paidTerms: number = +process.env.MEMBERSHIP_PAID_TERMS!;


For all the functions inside the impl folder which are now the reusable unit, it might be better to have these scenario parameters be passed as arguments and the caller would read them from process.env

registerJoystreamTypes() is also called in each test setup, and I think the caller of the test only needs to do that one time, and might as well also create the api instance and pass that as argument as well.

Scenario parameters now read from environment in scenarios and are passed to the implementation as parameters.

Type registration and api instance are still test-specific. I've run into issues while trying move them up to the scenario level. Let me know if we should discuss it.

mnaamani · 2020-05-22T12:32:02Z

tests/network-tests/src/constantinople/tests/proposals/impl/electionParametersProposal.ts

+    const minVotingStake: BN = await apiWrapper.getMinVotingStake();
+
+    // Proposal stake calculation
+    const proposalStake: BN = new BN(200000);


There are some runtime configured minimum values for stake, we should probably read the value instead of picking a magic number?

There are no way to read the required stake values from API. The definition of the values is here: https://github.com/Joystream/joystream/blob/development/runtime-modules/proposals/codex/src/proposal_types/parameters.rs

mnaamani · 2020-05-22T12:37:03Z

tests/network-tests/src/constantinople/tests/proposals/impl/electionParametersProposal.ts

+    const newMinCouncilStake: BN = await apiWrapper.getMinCouncilStake();
+    const newMinVotingStake: BN = await apiWrapper.getMinVotingStake();
+    assert(
+      announcingPeriod.subn(1).eq(newAnnouncingPeriod),


A better approach here that will also avoid some mistakes and doing the correct assertions:

const proposedAnnouncingPeriod = announcingPeriod.subn(1);

then propose that value, so assertion becomes

assert(newAnnouncingPeriod.eq(proposedAnnouncingPeriod))

mnaamani · 2020-05-22T12:39:13Z

tests/network-tests/src/constantinople/tests/proposals/impl/evictStoraveProvider.ts

+    assert(await apiWrapper.isStorageProvider(sudo.address), `Account ${sudo.address} is not storage provider`);
+
+    // Proposal stake calculation
+    const proposalStake: BN = new BN(25000);


Again it would be better to use the required stake from runtime.

mnaamani · 2020-05-22T21:23:06Z

 FAIL  src/constantinople/tests/proposals/updateRuntimeTest.ts
 ✖ timeout!

  test: "  Upgrading the runtime test"
  expired: TAP

 FAIL  src/constantinople/tests/proposals/updateRuntimeTest.ts 1 failed of 12 15m
 ✖ timeout!

I couldn't really figure out why it failed.

I realize now, the reason it failed is because the Migrated event would not have been raised because I didn't prepare one with a higher spec version than runtime of the test chain before updating it.

As discussed, chainspec is created from dev, so `purge-chain --dev` do the same. Co-authored-by: Mokhtar Naamani <[email protected]>

gleb-urvanov added 14 commits April 28, 2020 20:37

moved proposals tests to constantinople folder

2cd4010

Merge branch 'migration-constantinople' into feature/new-proposals-tests

d125bb2

removed rome tests, introduced assertions for new proposal tests

96bdcec

validator count proposal test implemented

5527844

set lead proposal test implemented

20ff033

build script for network tests added, readme updated

5ac4c2a

Merge branch 'feature/build-script' into feature/proposals-test-coverage

3c35b7d

evict storage provider proposal test implemented

3e0d381

set role parameters test implementation

9328be9

Merge branch 'development' of https://github.com/Joystream/substrate-…

8ba80f2

…runtime-joystream into development

Merge branch 'development' into feature/proposals-test-coverage

ad1b0be

storage provider parameters proposal test

52e2dd0

Constantinople proposals covered with network tests

239c80c

added periods alteration to the test

d2b986d

gleb-urvanov requested a review from mnaamani May 7, 2020 13:59

composed scenarios as sequence of functions

7138f79

This was referenced May 19, 2020

Feature/new proposals tests #322

Closed

build script for network tests added, readme updated #386

Closed

mnaamani reviewed May 19, 2020

View reviewed changes

gleb-urvanov added 3 commits May 19, 2020 11:54

merged development

387b981

refactored to use node-tap

644ea80

refactored to use node-tap instead of mocha

ff42ae6

gleb-urvanov marked this pull request as draft May 19, 2020 15:19

code refactoring, split test code into test and test function impleme…

f0795a4

…ntation

gleb-urvanov marked this pull request as ready for review May 20, 2020 09:48

Lezek123 pushed a commit to Lezek123/substrate-runtime-joystream that referenced this pull request May 21, 2020

Merge pull request Joystream#399 from Lezek123/fetch-cancellation-fee

cd81937

Cancellation fee: fetch from rutime

mnaamani requested changes May 22, 2020

View reviewed changes

gleb-urvanov added 2 commits May 26, 2020 09:11

Merge branch 'development' into feature/proposals-test-coverage

2d44c1d

testnet script minor fix

0d95545

made test implementation less opinionated

03a16a2

gleb-urvanov marked this pull request as draft May 27, 2020 16:08

gleb-urvanov and others added 8 commits May 28, 2020 00:20

introduced timeout calculation

114145f

changed timeout on evict storage provider test, small typo fixed

b3f42a6

assertions refactored

28e10a1

run test chain script fixed to work with mac

25b8097

.tmp folder for chainspec

d95bde2

Update scripts/run-test-chain.sh

2bb49e6

As discussed, chainspec is created from dev, so `purge-chain --dev` do the same. Co-authored-by: Mokhtar Naamani <[email protected]>

reverted accidental chainge

2f11cb8

code cleaning

d4dbc31

gleb-urvanov marked this pull request as ready for review May 28, 2020 12:57

merged development

1109153

mnaamani merged commit 8cca867 into Joystream:development Jun 1, 2020

	"@joystream/types": "../joystream-apps/packages/joy-types",
	"@joystream/types": "^0.9.1",

	sed -i 's/"runtimeUpgradeProposalGracePeriod":.*/"runtimeUpgradeProposalGracePeriod": 0,/' ./chainspec.json
	sed -i 's/"runtimeUpgradeProposalGracePeriod":.*/"runtimeUpgradeProposalGracePeriod": 0,/' chainspec.json

	sed -i 's/"setElectionParametersProposalGracePeriod":.*/"setElectionParametersProposalGracePeriod": 0,/' ./chainspec.json
	sed -i 's/"setElectionParametersProposalGracePeriod":.*/"setElectionParametersProposalGracePeriod": 0,/' chainspec.json

		@@ -0,0 +1,13 @@
		#!/bin/bash
		cargo run --release -p joystream-node build-spec --chain dev > chainspec.json

Feature/proposals test coverage #399

Feature/proposals test coverage #399

Uh oh!

Conversation

gleb-urvanov commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gleb-urvanov commented May 7, 2020 •

edited

Loading

mnaamani commented May 22, 2020 •

edited

Loading