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

Use self-consistent coverage configurations for separate runs #1092

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

area
Copy link
Member

@area area commented Sep 28, 2022

We've seen this issue in a lot of builds lately. It was intermittent, though, and so tough to pin down. I was also unable to reproduce locally. It ended up being due to this issue now that we're on Node 14 (and I'm attributing the inablility to reproduce locally due to node/npm version weirdness I've got going on locally).

I ended up forking istanbul-combine to use a fixed istanbul under the hood, which seems to solve the issue.

Unfortunately, the resulting package-lock.json threw an error when using npm ci (though not npm i, weirdly). Updating npm for the check-coverage stage of the build fixes that.

@chmanie
Copy link
Member

chmanie commented Sep 29, 2022

I'm taking a look at this. The complete overhaul of the package-lock.json seems a bit suspicious.

@area area force-pushed the maint/istanbul-nyc branch from 1ce385f to 7109a84 Compare September 30, 2022 13:02
@area area changed the title Use custom istanbul-combine Use self-consistent coverage configurations for separate runs Sep 30, 2022
@area
Copy link
Member Author

area commented Sep 30, 2022

Taking another swing at this. Diving down in to the guts, it turns out there was a race condition, but the warning being shown was unrelated. I do still think this was triggered somehow by Node 14, but at this point I just want out rather than understanding why 😛

Because some of our coverage instrumented the always modifier, and some didn't, we ended up with coverage runs for the same file (e.g. Colony.sol) that had different numbers of branch points. If one with fewer branchpoints was the first to be merged, then when the time came to merge the one with more branchpoints there was an error silently thrown in istanbul.

To fix this, I've inhereted the 'base' config where possible for solidity-coverage across all our runs, and changed the configuration where appropriate (primarily, the output directory).

We still have the warning about the circular dependency due to node 14, but that doesn't seem to be affecting anything with this change, so happy to leave it as-is.

@kronosapiens kronosapiens merged commit 2bb717e into develop Sep 30, 2022
@kronosapiens kronosapiens deleted the maint/istanbul-nyc branch September 30, 2022 15:35
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.

3 participants