Skip to content

Dont do coverage for PRs#4266

Merged
Kami merged 14 commits into
masterfrom
dont-do-coverage-for-prs
Jul 25, 2018
Merged

Dont do coverage for PRs#4266
Kami merged 14 commits into
masterfrom
dont-do-coverage-for-prs

Conversation

@blag

@blag blag commented Jul 24, 2018

Copy link
Copy Markdown
Contributor

Separated out from #4264 to ensure the CircleCI tests pass.

@blag blag changed the title Dont do coverage for prs Dont do coverage for PRs Jul 25, 2018
Comment thread Makefile Outdated
# NOTE: We only run coverage on master and version branches and not on pull requests since
# it has a big performance overhead and is very slow.
ifeq ($(TRAVIS_PULL_REQUEST),false)
ENABLE_COVERAGE := yes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks 👍

But as discussed on Slack, I will try to move this step (setting up the environment variable) to travis.yml file.

@Kami Kami added this to the 2.9.0 milestone Jul 25, 2018
@Kami

Kami commented Jul 25, 2018

Copy link
Copy Markdown
Member

@blag I pushed the environment variable change (1970613) which you didn't implement and we discussed on Slack and the other issue.

I will merge it into master and confirm it works correctly.

After that, we still need to fix support for coveralls.io reporting which was broken by the PR which refactored coverage stuff (asked discussed on Slack and the other issue).

@Kami Kami merged commit cd838e9 into master Jul 25, 2018
@Kami Kami deleted the dont-do-coverage-for-prs branch July 25, 2018 10:10
@Kami

Kami commented Jul 25, 2018

Copy link
Copy Markdown
Member

Confirmed it's working correctly for PRs (https://travis-ci.org/StackStorm/st2/builds/407977396) and for master (https://travis-ci.org/StackStorm/st2/builds/407994065).

This also confirmed again that coverage has massive performance overhead ~20 vs ~ 10 minutes so it's correct to only enable it for master and version branch (non pull request) builds.

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.

2 participants