Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for increased coverage #93

Merged
merged 1 commit into from Feb 11, 2020
Merged

Add tests for increased coverage #93

merged 1 commit into from Feb 11, 2020

Conversation

@ryanio
Copy link
Contributor

ryanio commented Feb 8, 2020

This PR adds tests for increased code coverage:

  • Adds BlockHeader validation test cases:
    • should validate a genesis block header
    • should validate a valid block header
    • should not validate a block header with invalid difficulty
    • should not validate a block header with invalid gas limit
    • should not validate a block header with invalid height
    • should not validate a block header with invalid timestamp
    • should not validate a block header with invalid parent
    • should not validate a header with invalid amount of extra data
  • Adds test for blockHeaderFromRpc:
    • should create a block header with the correct hash
  • Moves testdata json files to their own subdir to unclutter the parent test dir
  • Updates karma to run in both headless firefox and chrome
  • Modernizes the test and coverage command
  • Normalizes the readme badge style
st.end()
})

t.test('should validate a genesis block header', st => {

This comment has been minimized.

Copy link
@ryanio

ryanio Feb 8, 2020

Author Contributor

The new tests added in this PR start here ... sorry for diff, I renamed the files from test/*.ts to test/*.spec.ts

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 11, 2020

Member

Do you know that renaming can also be done via git which should preserve the file comparison respectively did you try?

For now it's ok.

@@ -56,18 +55,19 @@
"@ethereumjs/config-prettier": "^1.1.1",
"@ethereumjs/config-tsc": "^1.1.1",
"@ethereumjs/config-tslint": "^1.1.1",
"@types/lru-cache": "^5.1.0",

This comment has been minimized.

Copy link
@ryanio

ryanio Feb 8, 2020

Author Contributor

I found I had to add this type because ethereumjs-blockchain was failing to build in the browser. This might be good motivation to introduce a karma test runner for that repo to catch this and future issues.

The error encountered in karma was:

node_modules/ethereumjs-blockchain/dist/cache.d.ts:2:22 - error TS7016: Could not find a declaration file for module 'lru-cache'.

'/Users/rg/dev/ethereumjs-block/node_modules/ethereumjs-blockchain/node_modules/lru-cache/index.js' implicitly has an 'any' type.

  Try `npm install @types/lru-cache` if it exists or add a new declaration (.d.ts) file containing `declare module 'lru-cache';`

2 import * as LRU from 'lru-cache';

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 11, 2020

Member

Yeah, I think we should have karma tests along all (actively developed) libraries.

@ryanio ryanio requested a review from holgerd77 Feb 10, 2020
Copy link
Member

holgerd77 left a comment

Nice, thanks Ryan for wandering with open eyes through the libraries, yes, we still have some cases with significant gaps in test coverage, good to have such a significant step up here.

Will approve and directly merge.

"karma-firefox-launcher": "^1.1.0",
"karma-tap": "^4.1.4",
"nyc": "^14.0.0",
"nyc": "^15.0.0",
"prettier": "^1.17.0",
"tape": "^4.0.3",
"ts-node": "^8.0.3",

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 11, 2020

Member

All config-related stuff until here ok.

@@ -284,7 +284,7 @@ export class BlockHeader {
}

if (utils.bufferToInt(parentBlock.header.number) + 1 !== utils.bufferToInt(this.number)) {
throw new Error('invalid heigth')
throw new Error('invalid height')
}

if (utils.bufferToInt(this.timestamp) <= utils.bufferToInt(parentBlock.header.timestamp)) {

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 11, 2020

Member

Ok, some error message corrections.

mainnet: require('./difficultyMainNetwork.json').tests,
ropsten: require('./difficultyRopstenConstantinople.json').tests,
mainnet: require('./testdata/difficultyMainNetwork.json').tests,
ropsten: require('./testdata/difficultyRopstenConstantinople.json').tests,
}
for (const chain in chainTestData) {
const testData = chainTestData[chain]

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 11, 2020

Member

Ok, mainly shifts to testdata directory.

let block = blockHeaderFromRpc(blockData)
st.ok(block.hash().compare(Buffer.from(blockData.hash)))
st.end()
})

This comment has been minimized.

Copy link
@holgerd77
st.end()
})

t.test('should validate a genesis block header', st => {

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Feb 11, 2020

Member

Do you know that renaming can also be done via git which should preserve the file comparison respectively did you try?

For now it's ok.

st.fail(error)
st.end()
}
})

This comment has been minimized.

Copy link
@holgerd77
st.ok(true)
st.end()
}
})

This comment has been minimized.

Copy link
@holgerd77
st.end()
}
})
})

This comment has been minimized.

Copy link
@holgerd77
@holgerd77 holgerd77 merged commit 42bbb5f into master Feb 11, 2020
14 checks passed
14 checks passed
coverage
Details
coverage
Details
lint
Details
lint
Details
test (8.x)
Details
test (8.x)
Details
test (10.x)
Details
test (10.x)
Details
test (12.x)
Details
test (12.x)
Details
test (13.x)
Details
test (13.x)
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
coverage/coveralls First build on addTests at 86.923%
Details
@holgerd77 holgerd77 deleted the addTests branch Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.