-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
Hi @ryanio, this is so so great, thanks 😊! This updates actually makes the whole library so much more approachable, just realized this for myself while scanning through the changes, since I didn't dig too deep into the Some first note: we currently seem to have a general problem with coverage, see this issue ethereumjs/ethereumjs-config#22. Beyond coverage is not working with these changes from the PR since the base config from the ./node_modules/nyc/bin/nyc.js npm run test and modifying the {
"extends": "@ethereumjs/config-nyc",
"include": [
"dist/**/*.js"
]
} This might be a slight drop - not sure yet, the latest reported number here was on 96%. I nevertheless think we can live with this regarding the scope of the changes here. Latest coverage report is also quite old (from February 2019) so this might very well due to new code additions and not due to the changes here in the test suite. So again: 👍 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is not touching any production code and is at the same time a great base for further improvements or test additions, I would actually directly give this a go, have skimmed (roughly) through all the code parts.
Eventually give this another 12-24 hours if someone else wants to have an additional look, otherwise feel free to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ryan! This makes the tests much more clear and easier to analyze. Reviewed roughly half, have to leave now.
test/index.ts
Outdated
blockchain.getBlocks(genesisBlock.hash(), 5, 1, false, (err?: any, getBlocks?: any) => { | ||
st.error(err, 'no error') | ||
st.equal(getBlocks.length, 5) | ||
st.ok(getBlocks[0].header.number.equals(blocks[0].header.number)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found #137 when reading this. Can you please add a few more assertions that check which blocks should be returned fromgetBlocks
?
Hi @s1na, thanks for rescuing me here, realizing that my looking-over was too superficial. 🤨 |
…eBlockchain` * Updates skip behavior to skip from beginning (closes #137) * Running into weird edge case that a newly initialized Blockchain is returning 1 block. Added a failing test case: `should start with zero blocks`
Thanks for your comments @s1na! I am running into a weird edge case that a newly initialized test('blockchain test', function (t) {
const b = new Blockchain({ validateBlocks: false, validatePow: false })
b.getBlocks(0, 5, 0, false, (err?: Error, blocks?: any) => {
console.log(`blocks.length: ${blocks.length}`)
t.equal(err!.message, 'No head found.', 'should return correct error')
t.equal(blocks, undefined, 'should not return any blocks')
t.end()
})
}) Result:
I adjusted the |
@ryanio Can you update the branch here respectively rebase? |
You're right Ryan, it seems that ethereumjs-blockchain/src/index.ts Lines 264 to 265 in 5edfd80
It might be a bit confusing but I don't see a harm in it. Still we can consider changing it if there's consensus |
Maybe in a first iteration this can be just a bit better documented? |
Thanks @s1na for investigating! Ok I'm thinking I'll update this PR to just refactor the test suite as the library is currently written and add some additional clarification in the documentation. I'll open a separate PR for removing skip if we don't find anyone using it. |
Ok! Should be good to go now :) @s1na I would appreciate a final check that the tests look good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. Had a quick look at everything. Looks good!
I think there's room for another round of refactoring after the promisification to remove the async callbacks etc.
Just a small general comment for future PRs: the PR would've been easier to review if the test values (e.g. num of blocks added to the blockchain, etc.) hadn't been modified unless really necessary. Because each value modification requires us to analyse if the testcase is still testing the edge case it was supposed to.
st.error(err, 'should delete blocks in canonical chain') | ||
st.equals( | ||
blockchain._headHeader.toString('hex'), | ||
blocks[5].hash().toString('hex'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused why the head header should be block 5. Wasn't considering that delBlock deletes also all of the children of a block, which makes sense.
Thanks a lot for this change @ryanio! I didn't have enough time to review it, but it's a great improvement. I was always scared of changing these tests. |
This PR addresses #125 and #132 to refactor and modernize the test suite.
tape
test containing leaky shared state into individually siloed tests.Feedback and comments welcome! I am hoping this is a huge improvement.