refactor: test suite

[ryanio]

This PR addresses #125 and #132 to refactor and modernize the test suite.

1. Breaks up the single large tape test containing leaky shared state into individually siloed tests.
2. Updates the test fixture to one with metadata located here.

Feedback and comments welcome! I am hoping this is a huge improvement.

[holgerd77]

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 blockchain library and always had my problems to approach and experiment with the code. Within a holistic view I would also put stuff like this as a lower level part of a developer improvement initiative, I think for many of the more experienced devs this will make the library more approachable, really great! It also better prepares the library for future ongoing development and extension, if we e.g. extend the library to also support chains from the Eth 2.0 suite. We'll see. 😄

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 config library is set to work on the src folder. Think we need to give this some separate thinking, I just wanted to have the coverage results, so I ran this manually by runnning:

./node_modules/nyc/bin/nyc.js npm run test

and modifying the .nycrc file to:

{
  "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:

👍 😄

[holgerd77]

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.

[s1na]

Thanks Ryan! This makes the tests much more clear and easier to analyze. Reviewed roughly half, have to leave now.

[s1na]

Just found #137 when reading this. Can you please add a few more assertions that check which blocks should be returned fromgetBlocks?

[holgerd77]

Hi @s1na, thanks for rescuing me here, realizing that my looking-over was too superficial. 🤨

[ryanio]

Thanks for your comments @s1na!

I am running into a weird edge case that a newly initialized Blockchain returns 1 block. I added a failing test case. Any idea what could be going on here? I can recreate on master as well:

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:

$ npm test

> ethereumjs-blockchain@4.0.2 test /dev/ethereumjs-blockchain
> ts-node node_modules/tape/bin/tape ./test/index.ts

TAP version 13
# blockchain test
blocks.length: 1

/Users/ryanghods/dev/ethereumjs-blockchain/src/callbackify.ts:57
      return maybeCb.apply(self, arguments)
                     ^
TypeError: Cannot read property 'message' of null

I adjusted the getBlock tests as well according to your comments 👍

[holgerd77]

@ryanio Can you update the branch here respectively rebase?

[s1na]

You're right Ryan, it seems that Blockchain is purposefully creating a genesis block on initialization if one doesn't already exist:

ethereumjs-blockchain/src/index.ts

Lines 264 to 265 in 5edfd80

	// if genesis block doesn't exist, create one
	return self._setCanonicalGenesisBlock((err?: any) => {

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

[holgerd77]

You're right Ryan, it seems that Blockchain is purposefully creating a genesis block on initialization if one doesn't already exist:

ethereumjs-blockchain/src/index.ts

Lines 264 to 265 in 5edfd80

	// if genesis block doesn't exist, create one
	return self._setCanonicalGenesisBlock((err?: any) => {

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?

[ryanio]

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.

[ryanio]

Ok! Should be good to go now :)

@s1na I would appreciate a final check that the tests look good.

[s1na]

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.

[s1na]

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.

[alcuadrado]

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.