build_assertion error case test #1463

0xverin · 2023-03-15T08:29:12Z

resolves #1392

Kailai-Wang

I get the idea behind the tests, but I have a few comments about the coding (style).

Kailai-Wang · 2023-03-15T15:56:13Z

tee-worker/ts-tests/identity.test.ts

-        const [resp_substrate] = (await createIdentities(context, context.defaultSigner[0], aesKey, true, [substrateIdentity])) as IdentityGenericEvent[];
+        const [resp_substrate] = (await createIdentities(context, context.defaultSigner[0], aesKey, true, [
+            substrateIdentity,
+        ])) as IdentityGenericEvent[];


please feel free to adjust .prettierrc if you believe the old one has better readability

Kailai-Wang · 2023-03-15T16:02:24Z

tee-worker/ts-tests/identity.test.ts

+    step('remove error identities', async function () {
+        //remove a nonexistent identity from an account
+        const resp_not_exist_identities = (await removeErrorIdentities(context, context.defaultSigner[0], true, [
+            twitterIdentity,
+            ethereumIdentity,
+            substrateIdentity,
+        ])) as string[];
+
+        resp_not_exist_identities.map((item: any) => {
+            const result = item.toHuman().data.reason;
+            assert(
+                result.search('IdentityNotExist') !== -1,
+                'remove twitter should fail with reason `IdentityNotExist`'
+            );
+        });
+
+        //remove a challenge code before the code is set
+        const resp_not_created_identities = (await removeErrorIdentities(context, context.defaultSigner[2], true, [
+            twitterIdentity,
+            ethereumIdentity,
+            substrateIdentity,
+        ])) as string[];
+
+        resp_not_created_identities.map((item: any) => {
+            const result = item.toHuman().data.reason;
+            assert(
+                result.search('IdentityNotExist') !== -1,
+                'remove twitter should fail with reason `IdentityNotExist`'
+            );
+        });
+    });
+


This step seems identical with the previous one, we should have comment at least about why we repeat it

even though we repeat it - we should have a different step name

we should avoid duplicate code on step-level

we should avoid duplicate code inside a step: I don't see any difference between const resp_not_exist_identities... and const resp_not_created_identities... except for the signer. Can't we use a function to wrap it up?

The difference between the two removeErrorIdentities is that there is no challengeCode for context.defaultSigner[2],so for the convenience of testing, I directly change the signer and repeat it.

Yes I know the signer is different :) I mean we should point it out in the comment and try to remove duplicate code.

Kailai-Wang · 2023-03-15T16:04:46Z

tee-worker/ts-tests/indirect_error_calls.ts

+
+    for (let k = 0; k < indexList.length; k++) {
+        const tx = context.substrate.tx.vcManagement.revokeVc(indexList[k]);
+        const nonce = await context.substrate.rpc.system.accountNextIndex(signer.address);


A small improvement: I think we can move it out of the loop - we'll get the same result each time anyway.

Kailai-Wang · 2023-03-15T16:04:56Z

tee-worker/ts-tests/indirect_error_calls.ts

+
+    for (let k = 0; k < indexList.length; k++) {
+        const tx = context.substrate.tx.vcManagement.disableVc(indexList[k]);
+        const nonce = await context.substrate.rpc.system.accountNextIndex(signer.address);


Kailai-Wang · 2023-03-15T16:20:22Z

tee-worker/ts-tests/vc.test.ts

One of my general feelings here is (you might have other opinions):

We want to cover as many test cases as possible, but we don't want to over-test them. For example, we want to test the error handling of the case where a user requests a VC without setting the shielding key. This is a good test case, but we don't have to repeat it for all VC types that we have - because there isn't any difference in terms of such error handling for different assertion enums (if we have one someday, then we should test it).

So it doesn't make much difference you test A1 only vs you test A1 - A11, one VC type is enough. Making it a vector of VCs impairs the code readability and prolongs the whole test time unnecessarily.

The same applies to the identity test.

My idea is to select a random assertion to trigger the specified error, I don't know the difference between them, I worry that it will not cover all case(maybe A1 trigger,but A2 not trigger)

select a random assertion

But if I understand the code correctly, we iterate over all assertions here (instead of choosing one randomly). And yes there's no difference in terms of that specific error handling, we can ask eric or zhouhui if we are unsure.

I'm mainly concerned about the execution time - it probably won't be such a problem if a step takes 5s. Imagine we have 100 assertions later, we can't afford to test them all for a generic handling that applies to all assertion types (we should still do individual test if something differs)

I have confirmed with @zhouhuitian that only one test is needed, and all the procedures are the same,so only use A1 to triggers the error events.

Kailai-Wang

Looks better, thanks.

There're many formatting problems though, please fix it before committing.

Btw I don't think these .ts files went through prettier.

Kailai-Wang · 2023-03-16T15:20:10Z

tee-worker/ts-tests/identity.test.ts

+     step('remove prime identity NOT allowed', async function () {
+           // create substrate identity


Indention problem?

Kailai-Wang · 2023-03-16T15:20:25Z

tee-worker/ts-tests/identity.test.ts

        expect(events.length).to.be.equal(1);
        const result = events[0].method as string;
-    });
+     });


Kailai-Wang · 2023-03-16T15:20:33Z

tee-worker/ts-tests/identity.test.ts

-            ethereumIdentity,
-            substrateIdentity,
-        ])) as string[];
+        const identites = [twitterIdentity, ethereumIdentity, substrateIdentity];


typo: identities

Kailai-Wang · 2023-03-16T15:22:19Z

tee-worker/ts-tests/indirect_error_calls.ts

+
+
+


too many line breakers :(

Kailai-Wang · 2023-03-16T15:23:19Z

tee-worker/ts-tests/vc.test.ts

+
+
+// it doesn't make much difference test A1 only vs test A1 - A11, one VC type is enough.
+//So only use A1 to trigger the wrong event


extra line breakers

comments: first line with space, second line without

Kailai-Wang · 2023-03-16T23:07:50Z

And please resolve any conversation that is already addressed.

0xverin · 2023-03-17T03:46:16Z

I finally found where my local formatter configuration was wrong, my formatter was conflicting.:(

* build_assertion error propagate to parachain (#1456) * AssertionBuildFail: add more details * handle errors in on_success method * emit event when vc is non-exists * remove event * remove VCNotExist from Event * Fix benchmarking pallet-teerex (#1460) * remove tee-dev from node default feature * only rococo has tee-dev * remove sccache in docker * missing RUN * update Dockerfile * sleep for 1 second before the event websocket reconnect(fix #1416) (#1462) * debug: Sync of Moonbeam upstream debug (#1450) * debug: change of test * debug: fix benchmark * Fix the panic from WeightInfo in CI (#1466) * update comment * update runner tag * minor update, cleanup history data * fix errors in teeracle * [benchmarking bot] Auto commit generated weights files (#1465) Co-authored-by: Kailai-Wang <[email protected]> * using pallet weights --------- Co-authored-by: BillyWooo <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kailai-Wang <[email protected]> * build_assertion error case test (#1463) * error verify * batch call of identity * batch call of vc * format code * delete useless code * verify error identity * create error identities * delete useless code * delete useless code * add comment&delete useless code * format code * txs types * step error vc * catch dispatchError * revoke error test * requestVc error * change assert log * fix merge errors * add checkFailReason * check fail reason * move nonce out loop * format code * change test flow(only use A1 trigger error event) * format code * IDGraph adjustment (#1478) * only keep idgraph for verified event * add get_id_graph_with_max_len * add tee pallets unittests * adjust idgraph and prettier * use context.api for better naming * remove empty lines --------- Co-authored-by: Zhouhui Tian <[email protected]> Co-authored-by: Kai <[email protected]> Co-authored-by: Linfeng.Yuan <[email protected]> Co-authored-by: BillyWooo <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kailai-Wang <[email protected]> Co-authored-by: Verin1005 <[email protected]>

0xverin added 18 commits March 7, 2023 15:00

error verify

924deee

batch call of identity

8e5484a

batch call of vc

0818632

format code

658cb9b

delete useless code

7eafecf

verify error identity

820ac8a

create error identities

6be62b8

delete useless code

c46db24

delete useless code

8013596

add comment&delete useless code

8e55e0d

format code

363bad8

txs types

8c5da1c

step error vc

e092369

catch dispatchError

11f48cd

Merge branch 'dev' into 1392-build-assertion

48fd295

revoke error test

8b4f9d1

Merge branch 'dev' into 1392-build-assertion

f70a3fe

requestVc error

9f1079c

0xverin requested review from Kailai-Wang, jingleizhang and outofxxx March 15, 2023 08:29

0xverin force-pushed the 1392-build-assertion branch from a6017a0 to 0141966 Compare March 15, 2023 09:14

0xverin added 2 commits March 15, 2023 19:27

change assert log

25eeedf

fix merge errors

30e0998

0xverin force-pushed the 1392-build-assertion branch from 0141966 to 30e0998 Compare March 15, 2023 11:27

Kailai-Wang requested changes Mar 15, 2023

View reviewed changes

0xverin added 4 commits March 16, 2023 16:20

add checkFailReason

22cfc31

check fail reason

f044ffa

move nonce out loop

c541348

format code

19145ac

change test flow(only use A1 trigger error event)

9e77c3b

0xverin requested a review from Kailai-Wang March 16, 2023 14:53

Kailai-Wang approved these changes Mar 16, 2023

View reviewed changes

Kailai-Wang assigned 0xverin Mar 16, 2023

format code

2cfc52f

0xverin force-pushed the 1392-build-assertion branch from 48babc3 to 2cfc52f Compare March 17, 2023 03:17

0xverin merged commit f4d9572 into dev Mar 17, 2023

0xverin deleted the 1392-build-assertion branch March 17, 2023 07:18

		step('remove prime identity NOT allowed', async function () {
		// create substrate identity



		// it doesn't make much difference test A1 only vs test A1 - A11, one VC type is enough.
		//So only use A1 to trigger the wrong event

Uh oh!

build_assertion error case test #1463

build_assertion error case test #1463

Uh oh!

Conversation

0xverin commented Mar 15, 2023

Uh oh!

Kailai-Wang left a comment

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!

0xverin Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kailai-Wang Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kailai-Wang left a comment

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!

Kailai-Wang commented Mar 16, 2023

Uh oh!

0xverin commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0xverin Mar 15, 2023 •

edited

Loading

Kailai-Wang Mar 15, 2023 •

edited

Loading