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

Unit test for cics program tree #197

Merged
merged 6 commits into from
Jan 9, 2025

Conversation

almightyrush
Copy link
Contributor

@almightyrush almightyrush commented Jan 8, 2025

What It Does
Initial unit test setup for vsce package and unit test for CICSProgramTree file

How to Test
To test hit command: npm run test

Review Checklist
I certify that I have:

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.73%. Comparing base (10fcac9) to head (002f20d).

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #197       +/-   ##
===========================================
- Coverage   93.76%   27.73%   -66.04%     
===========================================
  Files          75      147       +72     
  Lines         770     5218     +4448     
  Branches       42      916      +874     
===========================================
+ Hits          722     1447      +725     
- Misses         48     3771     +3723     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Hey @almightyrush
Thanks for working on unit tests for the VSCE. 🙏

Also, you should have write access if you'd like to contribute directly from a branch 🙏

LGTM! 😋
image

@davenice
Copy link
Contributor

davenice commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.94%. Comparing base (428012c) to head (e7e8513).
Report is 1 commits behind head on main.

Hey @almightyrush - I notice that this improvement in unit testing (yay) drops the reported coverage down from 94%ish down to 28%ish.

I haven't quite worked out why .... I would imagine 28% is closer to the truth. Are we scanning a lot more now? Or are we accidentally uploading files more often than we ought to? 🤔

@almightyrush almightyrush force-pushed the Unit-test-for-CICSProgramTree branch from e7e8513 to 13af478 Compare January 9, 2025 06:15
@almightyrush
Copy link
Contributor Author

almightyrush commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.94%. Comparing base (428012c) to head (e7e8513).
Report is 1 commits behind head on main.

Hey @almightyrush - I notice that this improvement in unit testing (yay) drops the reported coverage down from 94%ish down to 28%ish.

I haven't quite worked out why .... I would imagine 28% is closer to the truth. Are we scanning a lot more now? Or are we accidentally uploading files more often than we ought to? 🤔

Hey @davenice,
So earlier vsce package was excluded from the coverage analysis. So the report only included average coverage of cli and sdk package.
Now the coverage is enabled for vsce so it takes the average of all the packages. Which results in the drop of total coverage.

@almightyrush
Copy link
Contributor Author

Hey @almightyrush Thanks for working on unit tests for the VSCE. 🙏

Also, you should have write access if you'd like to contribute directly from a branch 🙏

LGTM! 😋 image

Hi @zFernand0,
Yes sure please provide me the access 🙏.

@almightyrush almightyrush force-pushed the Unit-test-for-CICSProgramTree branch from 13af478 to bf271fb Compare January 9, 2025 08:31
Signed-off-by: Rushabh Sojitra <[email protected]>
@davenice
Copy link
Contributor

davenice commented Jan 9, 2025

Hey @almightyrush Thanks for working on unit tests for the VSCE. 🙏
Also, you should have write access if you'd like to contribute directly from a branch 🙏

Hi @zFernand0, Yes sure please provide me the access 🙏.

I think @zFernand0 is saying that because we're committers now, you don't have to work on your own fork but instead can create branches on the main repo and create PRs from there, going forward 👍

@@ -13,7 +13,8 @@
"sourceMap": true,
"newLine": "lf",
"strictNullChecks": false,
"noUnusedLocals": false
"noUnusedLocals": false,
"skipLibCheck": true
Copy link
Contributor

Choose a reason for hiding this comment

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

@almightyrush - can you help me understand what skipLibCheck does - is it required for the tests to work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the typescript code compiles, it is including the node_modules, and there are some compile errors which fails the npm run build . So skipping the node_modules from compiling.

@davenice
Copy link
Contributor

davenice commented Jan 9, 2025 via email

Copy link
Contributor

@davenice davenice left a comment

Choose a reason for hiding this comment

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

I think it's a yes from me overall!

@zFernand0
Copy link
Member

zFernand0 commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.94%. Comparing base (428012c) to head (e7e8513).
Report is 1 commits behind head on main.

Hey @almightyrush - I notice that this improvement in unit testing (yay) drops the reported coverage down from 94%ish down to 28%ish.
I haven't quite worked out why .... I would imagine 28% is closer to the truth. Are we scanning a lot more now? Or are we accidentally uploading files more often than we ought to? 🤔

Hey @davenice, So earlier vsce package was excluded from the coverage analysis. So the report only included average coverage of cli and sdk package. Now the coverage is enabled for vsce so it takes the average of all the packages. Which results in the drop of total coverage.

I don't have a strong preference on whether we start including the vsce right away, or we continue to exclude the vsce until we feel that we have a good amount of coverage on it 😋

I think it's nice to see hte numbers gradually increase with every PR, though. 😋

@davenice
Copy link
Contributor

davenice commented Jan 9, 2025

Hey @davenice, So earlier vsce package was excluded from the coverage analysis. So the report only included average coverage of cli and sdk package. Now the coverage is enabled for vsce so it takes the average of all the packages. Which results in the drop of total coverage.

I don't have a strong preference on whether we start including the vsce right away, or we continue to exclude the vsce until we feel that we have a good amount of coverage on it 😋

I think it's nice to see hte numbers gradually increase with every PR, though. 😋

I think let's include it ... honestly it represents reality a bit better 😬 ....! (And as you say, it means we can monitor the numbers better!)

@davenice davenice merged commit 18afd7c into zowe:main Jan 9, 2025
16 checks passed
@davenice
Copy link
Contributor

davenice commented Jan 9, 2025

@zFernand0 - would you have liked us to have an issue for this? @almightyrush's brief was to prototype a test for one type so that other people could use it as inspiration! Perhaps that explanation is required to clarify the seemingly arbitrary unit test :-)

@AndrewTwydell AndrewTwydell deleted the Unit-test-for-CICSProgramTree branch January 9, 2025 15:04
@zFernand0 zFernand0 added the release-current Indicates that there is no new functionality being delivered label Jan 9, 2025
@zFernand0
Copy link
Member

@zFernand0 - would you have liked us to have an issue for this? @almightyrush's brief was to prototype a test for one type so that other people could use it as inspiration! Perhaps that explanation is required to clarify the seemingly arbitrary unit test :-)

No worries. And thank you for asking! 🙏

I think it is nice to have issues for PRs that we believe will be impactful to end-users, since the issue could provide more context/details about the problem we are trying to solve (or feature we are trying to implement).

However, I think it's fine to save ourselves some paperwork for infrastructure-like PRs or other tech-debt items that won't make their way into the changelog (or release notes on docs.zowe.org). 🙏

Again, thanks for asking! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-current Indicates that there is no new functionality being delivered
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants