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

Remove code coverage setup #11198

Merged
merged 4 commits into from
Nov 1, 2018
Merged

Remove code coverage setup #11198

merged 4 commits into from
Nov 1, 2018

Conversation

youknowriad
Copy link
Contributor

The Code Coverage was helpful at the early stages of the project where we didn't have tests. It's not anymore as it only counts JavaScript unit tests and we have more tests than that. The results in PR are also not reliable. So let's just remove it to have a lighter setup.

@youknowriad youknowriad added the [Type] Build Tooling Issues or PRs related to build tooling label Oct 29, 2018
@youknowriad youknowriad self-assigned this Oct 29, 2018
package.json Outdated
@@ -152,7 +151,7 @@
"check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2\" \"wp-scripts check-licenses --dev\"",
"precheck-local-changes": "npm run docs:build",
"check-local-changes": "( git diff -U0 | xargs -0 node bin/process-git-diff ) || ( echo \"There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!\" && exit 1 );",
"ci": "concurrently \"npm run lint\" \"npm run test-unit:coverage-ci\" \"npm run check-local-changes\"",
"ci": "concurrently \"npm run lint\" \"npm run check-local-changes\"",
Copy link
Member

Choose a reason for hiding this comment

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

test-unit:coverage-ci should still be here we just need to update it as follows:

"test-unit:coverage-ci": "npm run test-unit -- --ci --runInBand",

gziolo
gziolo previously requested changes Oct 29, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Can we leave the setup for test coverage but only don't use it with Travis? In other words, everything that is related to codecov tool. I still see value in being able to run npm run test-unit:coverage locally.

@youknowriad
Copy link
Contributor Author

@gziolo My opinion is that it useless and just complexify the setup of the repository for nothing. Its results are not reliable and keeping it in the repository while not running in CI will more likely end up unmaintained.

@gziolo
Copy link
Member

gziolo commented Oct 29, 2018

I don't plan to fight for keeping this functionality. If more people find is useless we can remove it.

However run ci needs to run unit tests :) See: #11198 (comment)

@aduth
Copy link
Member

aduth commented Oct 29, 2018

I'd rather we not commit to maintaining it. It's likely to become neglected anyways if not part of the main pipeline.

@youknowriad youknowriad dismissed gziolo’s stale review October 31, 2018 14:17

The ci command should be fixed.

@@ -24,8 +24,6 @@ npm install @wordpress/jest-preset-default --save-dev

#### Brief explanations of options included

* `coverageDirectory` - the directory where Jest outputs its coverage files is set to `coverage`.
Copy link
Member

@gziolo gziolo Oct 31, 2018

Choose a reason for hiding this comment

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

We need to update changelog as this looks as a breaking change, or we can keep it as is 😄

I'm aware that probably nobody is using it.

@gziolo
Copy link
Member

gziolo commented Oct 31, 2018

LGTM, let's proceed when Travis is green.

@desrosj
Copy link
Contributor

desrosj commented Nov 1, 2018

Cleaned up the merge conflict and the tests are now passing. I can update the changelog as well if you feel it should be, @youknowriad. I was going to make the change as a suggestion on the PR, but I wasn't sure which changelog to update (the package's vs. the global one), and which version to list the change in, though. I am assuming the package version, but not sure.

@youknowriad
Copy link
Contributor Author

Thanks for your help @desrosj we should update the package CHANGELOG, I'm going to do it right now.

@youknowriad
Copy link
Contributor Author

This should be ready to go I need a ✅

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Sad to see this removed but happy at the same time to see our setup gets simplified

@youknowriad youknowriad merged commit 2388f17 into master Nov 1, 2018
@youknowriad youknowriad deleted the remove/codecov branch November 1, 2018 09:20
daniloercoli added a commit that referenced this pull request Nov 1, 2018
…rnmobile/port-quote-block-step-1

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Add removed periods to block descriptions. (#11367)
  Remove findDOMNode usage from the inserter (#11363)
  Remove deprecated componentWillReceiveProps from TinyMCE component (#11368)
  Create file blocks when dropping multiple files at once (#11297)
  Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168)
  Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180)
  Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342)
  Remove the Cloudflare warning (#11350)
  Image Block: Use source_url for media file link (#11254)
  Enhance styling of nextpage block using the Hr element (#11354)
  Embed block refactor and tidy (#10958)
  Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347)
  Fix RTL block alignments (#11293)
  RichText: fix buggy enter/delete behaviour (#11287)
  Remove code coverage setup (#11198)
  Parser: Runs all parser implementations against the same tests (#11320)
  Stop trying to autosave when title and classic block content both are empty. (#10404)
  Fix "Mac OS" typo + use fancy quotes consistently (#11310)
  Update documentation link paths (#11324)
  Editor: Reshape blocks state under own key (#11315)
  ...

# Conflicts:
#	gutenberg-mobile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants