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

Increase code coverage for repo-modenizer #40

Merged
merged 2 commits into from May 18, 2021

Conversation

dichaudhary
Copy link
Contributor

@dichaudhary dichaudhary commented May 18, 2021

Description

PR contains changes for a few additional test cases for repository modernizer module.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #40 (d1013e7) into master (4d7f6db) will increase coverage by 8.03%.
The diff coverage is n/a.

❗ Current head d1013e7 differs from pull request most recent head b8ccd6d. Consider uploading reports for the commit b8ccd6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   81.51%   89.54%   +8.03%     
==========================================
  Files          16       16              
  Lines        1569     1569              
  Branches      155      155              
==========================================
+ Hits         1279     1405     +126     
+ Misses        263      157     -106     
+ Partials       27        7      -20     
Impacted Files Coverage Δ
...itory-modernizer/src/util/pom-manipulation-util.js 88.63% <0.00%> (+71.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d7f6db...b8ccd6d. Read the comment docs.

@dichaudhary dichaudhary requested a review from ukgaurav May 18, 2021 03:41
@dichaudhary
Copy link
Contributor Author

The increase of 8% is on the overall repo & detailed increase can be seen here .
Also since the other modules contains private functions and one of the way to test them is using rewire (example)which currently has an issue integrating with code coverage (issue) result of which it is difficult to increase the coverage anyfurther.

Copy link
Member

@ManasMaji ManasMaji left a comment

Choose a reason for hiding this comment

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

Overall changes look great! Thanks @dichaudhary for working on increasing the coverage. 👍
Just fix the issue about the devDependencies vs dependencies.

@@ -20,6 +20,7 @@
},
"dependencies": {
"@adobe/aem-cs-source-migration-commons": "^0.0.3",
"axios-mock-adapter": "^1.19.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is this used library being anywhere? I could not find it.
Also incase it is required, it should be in the devDependencies list (libraries being used for development) instead of dependencies (libraries being used at runtime). Same with jest-junit, it should also be moved to devDependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @ManasMaji for pointing this out, Yes! it is not used anywhere, should have been included by mistake, let me remove it in the next commit.

Copy link
Contributor

@prateek610 prateek610 left a comment

Choose a reason for hiding this comment

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

LGTM

@dichaudhary dichaudhary merged commit 4d10d16 into adobe:master May 18, 2021
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.

None yet

3 participants