Skip to content

Conversation

@somniumism
Copy link
Contributor

@somniumism somniumism commented Mar 6, 2020

The tests were not updated when we changed the BlockSynMessage.
We removed the unnecessary score from the foundry and its tests.

resolves #208

@somniumism somniumism requested a review from majecty March 6, 2020 09:40
@somniumism
Copy link
Contributor Author

somniumism commented Mar 6, 2020

There are currently many test failures caused by time out as follows:

1) invalid block propagation 0
       "before each" hook for "OnChain invalid parent block propagation test":
     Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/somniumism/Project/PR/foundry/test/src/e2e.long/invalidBlockPropagation0.test.ts)
      at listOnTimeout (internal/timers.js:549:17)
      at processTimers (internal/timers.js:492:7)

@somniumism somniumism requested a review from junha1 March 6, 2020 10:14
@somniumism
Copy link
Contributor Author

somniumism commented Mar 6, 2020

I added @junha1 as a reviewer. He knows more about the foundry test and its structure than I do, so he can find a problem I can't find. Please check it carefully. : )))

Copy link
Contributor

@junha1 junha1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, those failures are not just timeouts. You must fix them all, and make sure they all passed in local first.

@kseo kseo added the do-not-merge Do not merge this PR label Mar 9, 2020
@somniumism
Copy link
Contributor Author

I found the cause of test failure. I separated the commit that causes it: Error: Remove the score from IStatus
The cause is related to SyncMessage. When this commit is applied, the following error occurs:

  invalid block propagation 0
Error: Expected 32 byte hexstring for creating H256 but found ""
    at new H256 (/Users/somniumism/Project/PR/foundry/test/node_modules/codechain-primitives/src/value/H256.ts:52:19)
    at Function.fromBytes (/Users/somniumism/Project/PR/foundry/test/src/helper/mock/blockSyncMessage.ts:66:30)
    at P2pLayer.onExtensionMessage (/Users/somniumism/Project/PR/foundry/test/src/helper/mock/p2pLayer.ts:326:55)
    at P2pLayer.onSignedMessage (/Users/somniumism/Project/PR/foundry/test/src/helper/mock/p2pLayer.ts:310:22)
    at Socket.<anonymous> (/Users/somniumism/Project/PR/foundry/test/src/helper/mock/p2pLayer.ts:138:46)
    at Socket.emit (events.js:321:20)
    at addChunk (_stream_readable.js:297:12)
    at readableAddChunk (_stream_readable.js:273:9)
    at Socket.Readable.push (_stream_readable.js:214:10)
    at TCP.onStreamRead (internal/stream_base_commons.js:186:23)
    1) "before each" hook for "OnChain invalid parent block propagation test"
    2) "after each" hook for "OnChain invalid parent block propagation test"

The error is related to the change in this code. I definitely removed totalScore from IStatus, so msg should contain only two elements. But it seems that there is still a totalScore in its 0th element. Once this error has been resolved, the work of removing the score from the foundry is finished.

@majecty Could you check what I did wrong and give me some advice?

@somniumism
Copy link
Contributor Author

There are 3 commits in this PR. I checked that the e2e test works successfully on Remove the score in the test.

@somniumism
Copy link
Contributor Author

somniumism commented Mar 9, 2020

It seems to be related to exchanging any data in the p2p layer. msg contains totalScore, bestHash, and genesisHash. I deleted totalScore, but I think there is still totalScore in the data exchanged. I don't know what data is exchanged or what it is.

I checked what the msg contains using console.log() as follows:

        if (msgId === MessageType.MESSAGE_ID_STATUS) {
            Emitter.emit("status");
            const msg = decodedmsg[1];
            console.log(msg[0]); //
            console.log(msg[1]); //
            console.log(msg[2]); //
            const totalScore = new U256(
                parseInt(msg[0].toString("hex"), 16) || 0
            );
            const bestHash = new H256(msg[1].toString("hex"));

The results are as follows:
Case 1) The commit: Remove the score in the test
(That is the case where the test works well)

<Buffer >
<Buffer 5d ef 65 01 00 3b 7d b7 4d 4b ca 8c 76 1a 37 35 ad c9 dc 22 69 39 88 5d 9f e1 d0 58 78 3b 29 79>
<Buffer 5d ef 65 01 00 3b 7d b7 4d 4b ca 8c 76 1a 37 35 ad c9 dc 22 69 39 88 5d 9f e1 d0 58 78 3b 29 79>

case 2) The commit: [Error] Remove the score from IStatus
(This is the case where a test failure occurs)

<Buffer >
<Buffer 5d ef 65 01 00 3b 7d b7 4d 4b ca 8c 76 1a 37 35 ad c9 dc 22 69 39 88 5d 9f e1 d0 58 78 3b 29 79>
<Buffer 5d ef 65 01 00 3b 7d b7 4d 4b ca 8c 76 1a 37 35 ad c9 dc 22 69 39 88 5d 9f e1 d0 58 78 3b 29 79>

I certainly did remove the total score in the second but in both cases the same result.

@somniumism somniumism requested review from junha1 and removed request for junha1 March 10, 2020 01:19
@somniumism somniumism changed the title [WIP] Remove the score in the foundry Remove the score in the foundry Mar 10, 2020
@somniumism somniumism added enhancement New feature or request and removed do-not-merge Do not merge this PR labels Mar 10, 2020
@somniumism somniumism requested review from junha1 and removed request for junha1 March 10, 2020 08:19
@junha1
Copy link
Contributor

junha1 commented Mar 10, 2020

@somniumism Need rebase

@somniumism
Copy link
Contributor Author

@junha1 I rebased it onto master.

@somniumism somniumism requested a review from junha1 March 10, 2020 09:35
junha1
junha1 previously approved these changes Mar 10, 2020
@junha1
Copy link
Contributor

junha1 commented Mar 10, 2020

This commit is very sensitive to e2e tests, so please merge it after it passes CI.

@junha1
Copy link
Contributor

junha1 commented Mar 11, 2020

@somniumism Could you check the error?

majecty
majecty previously approved these changes Mar 11, 2020
SeungMin Lee added 3 commits March 11, 2020 10:56
In the past, the work of removing the totalScore from the
BodySyncMessage and introducing the seq, the test was not modified at
that time. Therefore, we removed the remaining totalScore and changed it
to the seq.
@mergify mergify bot merged commit 55704ac into CodeChain-io:master Mar 11, 2020
@somniumism somniumism deleted the remove branch March 11, 2020 06:24
@MSNTCS MSNTCS mentioned this pull request Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the score in the Header and Block field

4 participants