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

Fix failing code-coverage command #4892

Merged
merged 8 commits into from
Mar 10, 2020
Merged

Conversation

Fawke
Copy link
Contributor

@Fawke Fawke commented Feb 25, 2020

Type of change

  • Other (Fix for gulp code coverage command)

Description of change

Commandsgulp test-coverage -> gulp view-coverage are not generating code coverage reports after updating npm packages.
Screenshot 2020-02-25 at 6 46 07 PM

The fix is to replace module karma-coverage-istanbul-reporter with karma-coverage.

@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2020

This pull request introduces 1 alert when merging 222ecdf into ac0ce27 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@jsnellbaker jsnellbaker added needs 2nd review Core module updates require two approvals from the core team needs review labels Feb 26, 2020
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @Fawke

Thanks for putting this together. It successfullly resolved the coverage issues I was experiencing recently when I updated my packages. I did have a question though, please see below when you have the chance.

Thanks.

Comment on lines 152 to 158
coverageReporter: {
dir: 'build/coverage',
reporters: [
{ type: 'html', subdir: 'karma-html' }
]
},

Copy link
Collaborator

Choose a reason for hiding this comment

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

As a question, I noticed with this block of code added here (and potentially at the reporters variable above), we now seem to always create this coverage directory (such as when we run gulp serve). Is this intended and needed? Or can we alter the code some-what to only generate these folders and files when the test-coverage command is ran?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsnellbaker Thanks for the feedback. That was a good catch, I shouldn't have gotten rid of if(codeCoverage) in function setReporters. I've added it back, and now it only generates coverage folder when we run the command, gulp test-coverage

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Works again thanks,

I do agree with @jsnellbaker if we can suppress the coverage dir when it is not asked for that would be nice.

@jsnellbaker
Copy link
Collaborator

Hi @Fawke

Thanks for resyncing the branch with master. It seems like there is one last thing to address.

When I tried to run the gulp coveralls command, I encountered an error:

[13:45:26] Starting 'coveralls'...
cat: build/coverage/lcov.info: No such file or directory
[error] "2020-03-09T17:45:26.550Z"  'error from lcovParse: ' 'Failed to parse string'
[error] "2020-03-09T17:45:26.552Z"  'input: ' ''
[error] "2020-03-09T17:45:26.553Z"  'error from convertLcovToCoveralls'

/Users/jsnellbaker/git/Prebid.js/node_modules/coveralls/bin/coveralls.js:19
      throw err;
      ^
Failed to parse string
[13:45:26] 'coveralls' errored after 456 ms
[13:45:26] Error in plugin 'gulp-shell'
Message:
    Command `cat build/coverage/lcov.info | node_modules/coveralls/bin/coveralls.js` failed with exit code 1
Details:
    domainEmitter: [object Object]
    domain: [object Object]
    domainThrown: false
[13:45:26] 'coveralls' errored after 1.18 min

Judging from the cat command, this new setup is not generating the output in a lcov.info file for coveralls to import.

I think if we update the reporter in the karma.conf to use lcov instead of html and update the paths in the gulp coveralls and gulp view-coverage commands, it should work. Can you give this a try?

@Fawke
Copy link
Contributor Author

Fawke commented Mar 10, 2020

Hi @jsnellbaker,

I did as you said, I changed the type to lcov from html and also changed the paths in gulpfile.js. But after doing that, I'm not getting the error you're getting, but a different error.

Screenshot 2020-03-10 at 1 42 17 PM

I read the read the RELEASE_SHEDULE.md file and found that you've to have a username and key to run this job. Since I don't have it, do you think that because of that I'm getting this error?

@jsnellbaker
Copy link
Collaborator

To note - we addressed permissions issue separately. This all appears to be working as expected now.

@jsnellbaker jsnellbaker merged commit 5d4c823 into master Mar 10, 2020
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Mar 13, 2020
* 'master' of https://github.com/prebid/Prebid.js: (49 commits)
  Submitting Clicktripz bid adapter (prebid#4929)
  add UNICORN bid adapter (prebid#4917)
  3.12.0-pre
  3.11.0 release
  Eids liveintent ext fix (prebid#4944)
  add mediaforce bid adapter (prebid#4933)
  update logic in adpod module for playersize (prebid#4953)
  Module - Size Mapping V2 (prebid#4690)
  Lifestreet adapter 3.0 (prebid#4927)
  IX Adapter - Increase banner TTL to 300s (prebid#4957)
  assert string returned not that we break things (prebid#4962)
  added option to url parser to ignore decoding entire url (prebid#4938)
  adding user-id support in medianet adapter (prebid#4925)
  removing the log (prebid#4960)
  add import extensions (prebid#4959)
  Add 7xbid adapter to compatible with prebid 3.0 (prebid#4908)
  Fix failing code-coverage command (prebid#4892)
  enable no-console eslint rule for project (prebid#4802)
  update audigent tests to fix larger test suite issue (prebid#4952)
  use bidId or bidIds in the payload (prebid#4903)
  ...
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Mar 13, 2020
* master: (49 commits)
  Submitting Clicktripz bid adapter (prebid#4929)
  add UNICORN bid adapter (prebid#4917)
  3.12.0-pre
  3.11.0 release
  Eids liveintent ext fix (prebid#4944)
  add mediaforce bid adapter (prebid#4933)
  update logic in adpod module for playersize (prebid#4953)
  Module - Size Mapping V2 (prebid#4690)
  Lifestreet adapter 3.0 (prebid#4927)
  IX Adapter - Increase banner TTL to 300s (prebid#4957)
  assert string returned not that we break things (prebid#4962)
  added option to url parser to ignore decoding entire url (prebid#4938)
  adding user-id support in medianet adapter (prebid#4925)
  removing the log (prebid#4960)
  add import extensions (prebid#4959)
  Add 7xbid adapter to compatible with prebid 3.0 (prebid#4908)
  Fix failing code-coverage command (prebid#4892)
  enable no-console eslint rule for project (prebid#4802)
  update audigent tests to fix larger test suite issue (prebid#4952)
  use bidId or bidIds in the payload (prebid#4903)
  ...
bmwcmw pushed a commit to criteo-forks/Prebid.js that referenced this pull request Mar 31, 2020
* replace karma-coverage-istanbul-reporter with karma-coverage

* remove unused variable

* create coverage folder when required

* merge with master to fix issue of all unit tests not running

* update processKillTimeout to 10secs

* change reporter type html to lcov
rjvelicaria pushed a commit to openx/Prebid.js that referenced this pull request Apr 9, 2020
* replace karma-coverage-istanbul-reporter with karma-coverage

* remove unused variable

* create coverage folder when required

* merge with master to fix issue of all unit tests not running

* update processKillTimeout to 10secs

* change reporter type html to lcov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants