Skip to content

Handle *.sol files that are invalid Solidity #156

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

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Conversation

area
Copy link
Contributor

@area area commented Nov 20, 2017

I introduced a nice bug in #147 which this fixes. In order to remove constant from all functions for instrumentation to take place, including those imported from ./node_modules, I ran the (increasingly badly named...) preprocessor over all .sol files that we find in the tree. Unfortunately, if we're using copyNodeModules:true, then that includes the solidity-coverage repository, which includes SimpleError.sol, which deliberately has a syntax error in it! This caused an uncaught error to be thrown, and solidity-coverage to fail to run.

I did think about just skipping SimpleError.sol itself, but don't want to be in the position where we break if any other package installed uses .sol files that aren't Solidity. I therefore leave any *.sol files that fail to parse alone, and generate a warning in case this is unexpected by the user. SimpleError.sol is now added to the truffle mock in order to prevent a regression along these lines.

Fixes #155

@area
Copy link
Contributor Author

area commented Nov 20, 2017

Check I've not done something silly again here @cgewecke, but otherwise I'm happy for this to be merged.

EDIT: I think coverage changes are because these tests aren't run on CI.

@codecov-io
Copy link

codecov-io commented Nov 20, 2017

Codecov Report

Merging #156 into master will decrease coverage by 0.5%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage    97.6%   97.09%   -0.51%     
==========================================
  Files           6        6              
  Lines         376      379       +3     
  Branches       86       86              
==========================================
+ Hits          367      368       +1     
- Misses          9       11       +2
Impacted Files Coverage Δ
lib/preprocessor.js 94.73% <93.33%> (-5.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 194a8aa...a1a55ef. Read the comment docs.

@cgewecke
Copy link
Member

cgewecke commented Nov 20, 2017

@area Yes looks good! What a weird problem. Merging and printing.

Side note: at the moment we have lost our E2E tests because Zeppelin is not upgrading their deps to the fork. Will the colonySale run clean on 0.4.x? We need a new (or additional) baseline.

@cgewecke cgewecke merged commit 4342743 into master Nov 20, 2017
@area
Copy link
Contributor Author

area commented Nov 20, 2017

Sorry, I don't understand why the Zeppelin contracts don't work?

@cgewecke
Copy link
Member

cgewecke commented Nov 20, 2017

Because their tests check for invalid opcode so most of them crash.
[EDIT] - Link to their CI for 0.3.5 which runs clean on the PR the testrpc maintainer opened for them, upgrading to 6.0

@area
Copy link
Contributor Author

area commented Nov 21, 2017

Very odd that they're not upgrading, especially with the PR's sat right there...

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

Successfully merging this pull request may close these issues.

solidity-coverage v0.4.1 conflicts with truffle v4.0.1
3 participants