-
Notifications
You must be signed in to change notification settings - Fork 765
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
ES5 distribution on npm #281
Conversation
…ith other ethereumjs libraries
…g 3rd-party build problems)
…point in package.json
… package.json script cmds to use --dist option
b4e3a38
to
12d2926
Compare
Update: have reverted this, makes more sense to run the state tests once on |
@Firescar96 reviewed this and gave his ok here, will take this as informal review and merge. |
@@ -4,7 +4,8 @@ os: | |||
|
|||
language: node_js | |||
node_js: | |||
- "6" |
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.
Why did we drop 6?
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.
Node 6 is not dropped, it's still below in the test matrix settings. Only the testBuildIntegrity
test is run with Node 8 and 9, all the other tests are run against Node 6. So all the test from before are still running with the same environment, with testBuildIntegrity
(just running a single state test) we are able to do additional checks without using too much of the limited run time for tests in travis.
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.
Cool, travis config can be misleading in cases, especially when the section shown by github is small :)
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.
So the goal of testBuildIntegrity
is to have a quick check with a single test?
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.
Yes, to catch stuff like "is Node 10" generally working, "is the dist folder existing", stuff like that.
@@ -55,7 +55,8 @@ const skipSlow = [ | |||
'static_Call1MB1024Calldepth', // slow | |||
'static_Callcode50000', // slow | |||
'static_Return50000', // slow | |||
'static_Return50000_2' // slow | |||
'static_Return50000_2', // slow | |||
'QuadraticComplexitySolidity_CallDataCopy' |
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.
Why was this removed?
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.
This test is extremely slow, travis is getting on its limits (sometimes).
"testBlockchainBlockGasLimit": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcBlockGasLimitTest'", | ||
"testBlockchainValid": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcValidBlockTest'", | ||
"testBlockchainTotalDifficulty": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcTotalDifficultyTest'", | ||
"test": "node ./tests/tester -a", |
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.
This is lacking build:dist
.
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.
Tests don't necessarily have to be run against the dist
folder, this can now be switched providing the dist
option. The test
command (which I think isn't used anyhow), is remaining on lib
(was undecided on this). We could generally make this more explicit by renaming test commands to e.g. testBlockchain:dist
, not sure if this is necessary though.
"dist" | ||
], | ||
"scripts": { | ||
"coverage": "istanbul cover ./tests/tester.js -- -s", |
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.
This is lacking build:dist
.
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.
No, it's not, this is explained above. I'll cite here:
"Update: After some experimentation it turned out that Coveralls showing the files covered couldn't be combined with the desired property of not having the dist folder in Git by excluding the folder in .gitignore. So coverage is now run against the lib folder. Which also makes sense."
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.
Ah I see, if --dist
is specified it runs on the dist
directory, otherwise on lib
.
"scripts": { | ||
"coverage": "istanbul cover ./tests/tester.js -- -s", | ||
"coveralls": "npm run coverage && coveralls <coverage/lcov.info", | ||
"testVM": "node ./tests/tester -v", |
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.
This is lacking build:dist
.
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.
VM tests are not active atm, this would only be pseudo-consistency.
"testBlockchainValid": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcValidBlockTest'", | ||
"testBlockchainTotalDifficulty": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcTotalDifficultyTest'", | ||
"test": "node ./tests/tester -a", | ||
"lint": "standard", |
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.
Will lint
ignore the dist
directory?
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.
Yes.
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.
Yeah I see the dist/**
line in apackage.json
, just the change view on github didn't made it apparent.
"testBlockchainTotalDifficulty": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='bcTotalDifficultyTest'", | ||
"test": "node ./tests/tester -a", | ||
"lint": "standard", | ||
"prepublishOnly": "npm run lint && npm run build:dist && npm run testBuildIntegrity", |
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.
Not sure if this will run build:dist
twice?
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.
Yes, it does, could probably be removed. The dist build is not taking that much time though, just a couple of seconds.
@axic General remark: ok, I see, won't use external reviews as full reviews any more. Thought that would suffice (since you guys are all pretty busy atm), sorry. Next time I'll request a classic team dev review again. |
In general I wanted to understand the changes, they all look ok after clarifications above. |
Addresses #271, (as discussed and agreed upon in the PR discussion) also alternative solution to PR #274 .
This PR uses
Babel
to build anES5
compatible distribution build in thedist
folder. Thedist/index.js
file is then referenced as the main entry point inpackage.js
. Thedist
folder itself is excluded from the repo (through.gitignore
) and just created before publish or testing.The normal test runs are still done against the
lib
folder. There is a new test option--dist
introduced, which is used for thepackage.json
test scripts and allow for testing against thedist
folder files.Since only thedist
folder is included in the npm build, coverage is now also run against thedist
folder and coveralls test report will show thedist
folder files. This shouldn't be that much of a problem though, since the structure of the files didn't change a lot (except some replacements ofconst
withvar
e.g.), so it should still be easily possible to identify the code parts missing code coverage.Update: After some experimentation it turned out that
Coveralls
showing the files covered couldn't be combined with the desired property of not having thedist
folder in Git by excluding the folder in.gitignore
. Socoverage
is now run against thelib
folder. Which also makes sense.One note for review: I also switched the order of some config elements in
package.json
to be more conform with other libraries. This was a bit unlucky since it hides a bit the relevant changes when just looking at the complete code base changes. Review can therefor better be done by looking through the single commit changes.