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

Write an istanbul verifyConditions plugin #68

Closed
boennemann opened this Issue Aug 23, 2015 · 13 comments

Comments

Projects
None yet
9 participants
@boennemann
Member

boennemann commented Aug 23, 2015

The popular code coverage generator istanbul offers the check-coverage command.

I'd love to see a semantic-release verifyConditions plugin (just like condition-travis) that aborts any release where defined coverage thresholds aren't met, but I might not immediately have the time to do it myself.

If you want to give this a shot let me know in this issue and I'm happy to help wherever I can. You can reach me in the semantic-release gitter room, or on Twitter.

@kentcdodds

This comment has been minimized.

Show comment
Hide comment
@kentcdodds

kentcdodds Sep 8, 2015

Contributor

For what it's worth, I actually use this as part of my scripts and pre-commit hooks so the build will break if a PR or commit doesn't have sufficient code coverage.

Contributor

kentcdodds commented Sep 8, 2015

For what it's worth, I actually use this as part of my scripts and pre-commit hooks so the build will break if a PR or commit doesn't have sufficient code coverage.

@ariporad

This comment has been minimized.

Show comment
Hide comment
@ariporad

ariporad Sep 8, 2015

Contributor

Me too. I think most code coverage tools have an option to fail if they
don't pass a certain threshold.

Ari

On Tue, Sep 8, 2015 at 4:12 AM, Kent C. Dodds notifications@github.com
wrote:

For what it's worth, I actually use this as part of my scripts and
pre-commit hooks so the build will break if a PR or commit doesn't have
sufficient code coverage.


Reply to this email directly or view it on GitHub
#68 (comment)
.

Contributor

ariporad commented Sep 8, 2015

Me too. I think most code coverage tools have an option to fail if they
don't pass a certain threshold.

Ari

On Tue, Sep 8, 2015 at 4:12 AM, Kent C. Dodds notifications@github.com
wrote:

For what it's worth, I actually use this as part of my scripts and
pre-commit hooks so the build will break if a PR or commit doesn't have
sufficient code coverage.


Reply to this email directly or view it on GitHub
#68 (comment)
.

@apowers313

This comment has been minimized.

Show comment
Hide comment
@apowers313

apowers313 Sep 18, 2015

Seeing as it's automated, I don't see any harm in doing it in semantic-release too.

Istanbul has a json-summary reporter that kicks out a report in the following format:

{
  "total": {
    "lines": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "statements": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "functions": {
      "total": 1,
      "covered": 1,
      "skipped": 0,
      "pct": 100
    },
    "branches": {
      "total": 0,
      "covered": 0,
      "skipped": 0,
      "pct": 100
    },
    "linesCovered": {
      "7": 3,
      "8": 3,
      "9": 3
    }
  },
  "/private/tmp/open-element-template/test.js": {
    "lines": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "statements": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "functions": {
      "total": 1,
      "covered": 1,
      "skipped": 0,
      "pct": 100
    },
    "branches": {
      "total": 0,
      "covered": 0,
      "skipped": 0,
      "pct": 100
    },
    "linesCovered": {
      "7": 3,
      "8": 3,
      "9": 3
    }
  }
}

Making this work would require:

  1. Using Istanbul for code coverage
  2. Ensuring that json-reporter was enabled as a reporter
  3. Having a configuration option for the plugin that points to the coverage directory
  4. Having a configuration option for the passing threshold

The semantic-report plugin would read in {{coverage}}/coverage-summary.json, compare {{threshold}} to the total coverage for lines, statements, functions and branches and return true if the coverage is greater than or equal to the threshold.

One question -- is there a way for a plugin to send a failure notification (email, slack, etc.) so that a minimal amount of time is spent trying to understand why a release didn't go out? Doesn't seem like every plugin should write their own notification mechanisms...

Seeing as it's automated, I don't see any harm in doing it in semantic-release too.

Istanbul has a json-summary reporter that kicks out a report in the following format:

{
  "total": {
    "lines": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "statements": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "functions": {
      "total": 1,
      "covered": 1,
      "skipped": 0,
      "pct": 100
    },
    "branches": {
      "total": 0,
      "covered": 0,
      "skipped": 0,
      "pct": 100
    },
    "linesCovered": {
      "7": 3,
      "8": 3,
      "9": 3
    }
  },
  "/private/tmp/open-element-template/test.js": {
    "lines": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "statements": {
      "total": 3,
      "covered": 3,
      "skipped": 0,
      "pct": 100
    },
    "functions": {
      "total": 1,
      "covered": 1,
      "skipped": 0,
      "pct": 100
    },
    "branches": {
      "total": 0,
      "covered": 0,
      "skipped": 0,
      "pct": 100
    },
    "linesCovered": {
      "7": 3,
      "8": 3,
      "9": 3
    }
  }
}

Making this work would require:

  1. Using Istanbul for code coverage
  2. Ensuring that json-reporter was enabled as a reporter
  3. Having a configuration option for the plugin that points to the coverage directory
  4. Having a configuration option for the passing threshold

The semantic-report plugin would read in {{coverage}}/coverage-summary.json, compare {{threshold}} to the total coverage for lines, statements, functions and branches and return true if the coverage is greater than or equal to the threshold.

One question -- is there a way for a plugin to send a failure notification (email, slack, etc.) so that a minimal amount of time is spent trying to understand why a release didn't go out? Doesn't seem like every plugin should write their own notification mechanisms...

@apowers313

This comment has been minimized.

Show comment
Hide comment
@apowers313

apowers313 Sep 21, 2015

Is the assumption for verifyConditions that if the callback has an error the condition fails; otherwise, the condition succeeds?

Is the assumption for verifyConditions that if the callback has an error the condition fails; otherwise, the condition succeeds?

@boennemann

This comment has been minimized.

Show comment
Hide comment
@boennemann

boennemann Sep 21, 2015

Member

Here are several open things and I'll try to address these now :)

  • I think it makes sense to run this only at release time and not during pre-commit, because this is exactly where it is relevant. Enforcing passing tests/sufficient coverage at commit time makes it impossible to push WIP things and let them run on CI, you can't push failing tests to prove they're failing, this is unfriendly for newcomers, who might get hindered from sending a PR in the first place etc.
  • Istanbul already has this built in:
Available commands are:

      check-coverage
              checks overall/per-file coverage against thresholds from coverage
              JSON files. Exits 1 if thresholds are not met, 0 otherwise

https://github.com/gotwarlost/istanbul#the-command-line

  • At this point I'm wondering if it makes sense to only implement a run-script plugin (#69) instead, because of how straight forward istanbul check-coverage already is
  • If the verifyConditions callback gets an error passed as the first argument it is considered a failure
  • I'm thinking of building a mechanism where the results of the verification get attached to the commit via the GitHub status API.

Best,
Stephan

Member

boennemann commented Sep 21, 2015

Here are several open things and I'll try to address these now :)

  • I think it makes sense to run this only at release time and not during pre-commit, because this is exactly where it is relevant. Enforcing passing tests/sufficient coverage at commit time makes it impossible to push WIP things and let them run on CI, you can't push failing tests to prove they're failing, this is unfriendly for newcomers, who might get hindered from sending a PR in the first place etc.
  • Istanbul already has this built in:
Available commands are:

      check-coverage
              checks overall/per-file coverage against thresholds from coverage
              JSON files. Exits 1 if thresholds are not met, 0 otherwise

https://github.com/gotwarlost/istanbul#the-command-line

  • At this point I'm wondering if it makes sense to only implement a run-script plugin (#69) instead, because of how straight forward istanbul check-coverage already is
  • If the verifyConditions callback gets an error passed as the first argument it is considered a failure
  • I'm thinking of building a mechanism where the results of the verification get attached to the commit via the GitHub status API.

Best,
Stephan

@apowers313

This comment has been minimized.

Show comment
Hide comment
@apowers313

apowers313 Sep 21, 2015

If you were to implement Istanbul as a verifyConditions script, wouldn't that mean that you would have to run the test suite twice -- once during Travis's npm test and once during semantic-release's verifyConditiions? If that's the case, it might be too much overhead for large test suites...

I like checking Istanbul coverage during semantic-release because 1) it won't mark the build as failing if coverage thresholds aren't met; 2) all of the release requirements are managed by a single decisioning engine, rather than trying to figure what is breaking and where; 3) it simply stops a release if coverage is insufficient, which seems like the most appropriate (and semantically correct) action.

If you were to implement Istanbul as a verifyConditions script, wouldn't that mean that you would have to run the test suite twice -- once during Travis's npm test and once during semantic-release's verifyConditiions? If that's the case, it might be too much overhead for large test suites...

I like checking Istanbul coverage during semantic-release because 1) it won't mark the build as failing if coverage thresholds aren't met; 2) all of the release requirements are managed by a single decisioning engine, rather than trying to figure what is breaking and where; 3) it simply stops a release if coverage is insufficient, which seems like the most appropriate (and semantically correct) action.

@kentcdodds

This comment has been minimized.

Show comment
Hide comment
@kentcdodds

kentcdodds Sep 21, 2015

Contributor

If I need a WIP commit, I simply commit with --no-verify and it skips the commit hooks. I actually prefer the build to break if the thresholds aren't met.

Sounds to me like this would be a fine micro-plugin. I personally wouldn't use it.

Contributor

kentcdodds commented Sep 21, 2015

If I need a WIP commit, I simply commit with --no-verify and it skips the commit hooks. I actually prefer the build to break if the thresholds aren't met.

Sounds to me like this would be a fine micro-plugin. I personally wouldn't use it.

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Jan 10, 2016

nyc will be a better choice as it also supports test runner which run tests in subprocesses.

nyc will be a better choice as it also supports test runner which run tests in subprocesses.

@boennemann

This comment has been minimized.

Show comment
Hide comment
@boennemann

boennemann Jan 13, 2016

Member

@satya164 nyc uses istanbul under the hood, so this will work with coverage reports nyc has reported, too.

Member

boennemann commented Jan 13, 2016

@satya164 nyc uses istanbul under the hood, so this will work with coverage reports nyc has reported, too.

@saiichihashimoto

This comment has been minimized.

Show comment
Hide comment
@saiichihashimoto

saiichihashimoto Sep 27, 2016

I'm really confused as to the value of this (or #67, for that matter). From what I gather, semantic-release is about automating package publishing. And in continuous integration, it makes sense for the build to fail before we reach this point if our tests fail. Don't we want the tests (and ultimately, the build) to fail if the coverage is too low?

Perhaps the purpose of semantic-release isn't defined well enough. From what I understand, semantic release:

  • Examines the package, determining if it's the right situation to release and, if it is, determining the version to release.
  • Doing the actual release
  • Creating documentation and git tags

It doesn't look like semantic-release should reinvent running tests. That can (and currently should be) run before semantic-release and fail the build if they don't pass. The exceptions to this would be running cracks or #65 but even those are running tests to determine what the next version should be: a job of semantic-release.

TLDR If coverage should abort the release, isn't the best way to fail the build, just as failing tests would?

I'm really confused as to the value of this (or #67, for that matter). From what I gather, semantic-release is about automating package publishing. And in continuous integration, it makes sense for the build to fail before we reach this point if our tests fail. Don't we want the tests (and ultimately, the build) to fail if the coverage is too low?

Perhaps the purpose of semantic-release isn't defined well enough. From what I understand, semantic release:

  • Examines the package, determining if it's the right situation to release and, if it is, determining the version to release.
  • Doing the actual release
  • Creating documentation and git tags

It doesn't look like semantic-release should reinvent running tests. That can (and currently should be) run before semantic-release and fail the build if they don't pass. The exceptions to this would be running cracks or #65 but even those are running tests to determine what the next version should be: a job of semantic-release.

TLDR If coverage should abort the release, isn't the best way to fail the build, just as failing tests would?

@phra

This comment has been minimized.

Show comment
Hide comment
@phra

phra Jun 5, 2017

@saiichihashimoto i'm exactly doing that. in an agile environment, every commit should be able to be promoted to master without disasters, so if the coverage is not correct, the commit itself should fail.

phra commented Jun 5, 2017

@saiichihashimoto i'm exactly doing that. in an agile environment, every commit should be able to be promoted to master without disasters, so if the coverage is not correct, the commit itself should fail.

@pvdlg

This comment has been minimized.

Show comment
Hide comment
@pvdlg

pvdlg Dec 15, 2017

Member

As mentioned in several comments running test coverage is more appropriate in the test phase than in the release phase. If we choose to enforce a certain percentage of coverage, and it's not met the test should fails and semantic-release release shouldn't even be called. In addition the alert can be reported directly in the PR (as the build would fail due to the failed tests), before it get merged.

As anyone any objection regarding closing this issue?

Member

pvdlg commented Dec 15, 2017

As mentioned in several comments running test coverage is more appropriate in the test phase than in the release phase. If we choose to enforce a certain percentage of coverage, and it's not met the test should fails and semantic-release release shouldn't even be called. In addition the alert can be reported directly in the PR (as the build would fail due to the failed tests), before it get merged.

As anyone any objection regarding closing this issue?

@saiichihashimoto

This comment has been minimized.

Show comment
Hide comment

Let's close it

@gr2m gr2m closed this Dec 31, 2017

@pvdlg pvdlg removed the info requested label Jan 1, 2018

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