Refactor test suite #231
Refactor test suite #231
Conversation
| @@ -1,4 +1,5 @@ | |||
| import BN = require('bn.js') | |||
| const Buffer = require('buffer').Buffer | |||
There was a problem hiding this comment.
I needed to include this for the source to compile correctly for browsers in the karma test runner.
|
Hi @ryanio, thanks for tackling this so quickly! 😄 Some initial notes: this PR is mixing up too much stuff, things should generally be addressed in a more atomic way for a PR to be better reviewable and also for - eventually necessary - later-on error/bug attribution. So these should rather be two (for the file split and the GitHub action transition) or eventually three (also for the Karma changes) PRs. Same a bit for the commits being done: changes are too big to pack this into a single commit, also for reasons of historical referenceability and again ease of review. The commit message along is also too generic to be meaningful enough. So there should be at least 4 separate commits attributed to the items identified above, eventually also some more and a bit more fine-grained. I e.g. discovered some implicit but significant changes (switch from Istanbul to nyc) hidden in the code base without any mentioning. Won't make all of this a blocker for economic reasons, just as a note for future PRs. Think in this case this is justifiable to some extend since all changes are at least in the same scope of tests and at the end this can also be seen as some larger refactoring on this front. Ok, so far. Apart from this all this is really a large improvement for the library, cool! |
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| node-version: [8.x, 10.x, 12.x, 13.x] |
There was a problem hiding this comment.
For the record: this PR removes support for Node 11 and adds support for Node 13.
| concurrency: Infinity, | ||
| // Fail after timeout | ||
| browserDisconnectTimeout: 100000, | ||
| browserNoActivityTimeout: 100000, |
There was a problem hiding this comment.
Could you add some notes on the config you changed here together with some reasoning along?
There was a problem hiding this comment.
I grabbed some of the sensible defaults that were being used in ethereumjs-vm/karma.conf.js. I was going to bring over the comments indicating what each line does but decided not to for brevity, next time I will. Thanks!
| "mocha": "^6.0.0", | ||
| "nyc": "^15.0.0", |
There was a problem hiding this comment.
For the record: switched from istanbul to nyc, dropped various babel and karma related dev dependencies.
| assert.equal(isValidAddress('0X52908400098527886E0F7030069857D2E4169EE7'), false) | ||
| assert.equal(isValidAddress('x2f015c60e0be116b1f0cd534704db9c92118fb6a'), false) | ||
| }) | ||
| }) |
| assert.equal(someOb.blah.toString('hex'), '01') | ||
| assert.equal(someOb.aword.toString('hex'), '01') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The stuff from defineFields.js, good.
| toRpcSig(1, r, s) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
6 new files for 6 category source files, ok.
|
Also made the switch from the Travis to the GitHub action tests run being required and also included Coveralls to be now required (together with some reasonable thresholds) along the way. Will now merge. |
This PR: