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

ES5 distribution on npm #281

Merged
merged 5 commits into from
Mar 6, 2018
Merged

ES5 distribution on npm #281

merged 5 commits into from
Mar 6, 2018

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Mar 1, 2018

Addresses #271, (as discussed and agreed upon in the PR discussion) also alternative solution to PR #274 .

This PR uses Babel to build an ES5 compatible distribution build in the dist folder. The dist/index.js file is then referenced as the main entry point in package.js. The dist 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 the package.json test scripts and allow for testing against the dist folder files.

Since only the dist folder is included in the npm build, coverage is now also run against the dist folder and coveralls test report will show the dist 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 of const with var 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 the dist folder in Git by excluding the folder in .gitignore. So coverage is now run against the lib 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.

@holgerd77 holgerd77 force-pushed the es5-dist branch 3 times, most recently from b4e3a38 to 12d2926 Compare March 1, 2018 15:21
@coveralls
Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage decreased (-0.009%) to 85.453% when pulling 416e559 on es5-dist into e863b93 on master.

@holgerd77
Copy link
Member Author

holgerd77 commented Mar 1, 2018

I've also integrated here state tests and coverage run on travis, so that we don't have to run the state tests twice.

Update: have reverted this, makes more sense to run the state tests once on dist folder and for coverage on lib, otherwise coveralls cannot map coverage to the source files.

@holgerd77 holgerd77 changed the title [WIP] ES5 distribution on npm ES5 distribution on npm Mar 1, 2018
@holgerd77 holgerd77 requested review from axic and jwasinger March 1, 2018 18:40
@holgerd77
Copy link
Member Author

@Firescar96 reviewed this and gave his ok here, will take this as informal review and merge.

@holgerd77 holgerd77 merged commit 9bc99c3 into master Mar 6, 2018
@holgerd77 holgerd77 deleted the es5-dist branch March 6, 2018 09:42
@@ -4,7 +4,8 @@ os:

language: node_js
node_js:
- "6"
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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 :)

Copy link
Member

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?

Copy link
Member Author

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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Member Author

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",
Copy link
Member

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.

Copy link
Member Author

@holgerd77 holgerd77 Mar 7, 2018

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",
Copy link
Member

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.

Copy link
Member Author

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."

Copy link
Member

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",
Copy link
Member

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.

Copy link
Member Author

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",
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

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",
Copy link
Member

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?

Copy link
Member Author

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.

@holgerd77
Copy link
Member Author

@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.

@axic
Copy link
Member

axic commented Mar 7, 2018

In general I wanted to understand the changes, they all look ok after clarifications above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants