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

Added tests for pkg/utils/gclient #5301

Conversation

anujagrawal699
Copy link
Contributor

@anujagrawal699 anujagrawal699 commented Aug 5, 2024

Description
This PR introduces unit tests for the gclient package to ensure its functionality and maintainability. Currently, the package lacks unit tests, making it difficult to catch potential regressions and verify its behavior. By adding these tests, we aim to improve the reliability and code coverage of this critical component.

The following tests are implemented:
a. TestNewSchema: Verify that NewSchema() returns a non-nil schema.
b. TestNewForConfig: Test NewForConfig() with both valid and invalid configurations.
c. TestNewForConfigOrDie: Test TestNewForConfigOrDieWithValidConfig() with a valid configuration.
d. TestAggregatedScheme: Verify that the aggregated scheme includes expected types.

Test Coverage
The gclient package lacked unit tests and thus implementing this tests increased it's test coverage from 0% to 94.7%. This can be verified in the file directory i.e. pkg/util/gclient using command
go test -coverprofile=coverage.out

What type of PR is this?
/kind failing-test
/kind feature

What this PR does / why we need it:
The tests introduced in this PR will:
Ensure the basic functionality of the gclient package
Improve test coverage of gclient package to 94.7%
Serve as documentation for expected behavior
Help catch potential issues in future changes

Which issue(s) this PR fixes:
Fixes a part #5236 and #5300

Special notes for your reviewer:
These commits are part of my introduction to the Karmada project and provide me with hands-on experience in implementing unit tests within the project. I look forward to implement more unit tests in the LFX Mentorship program.

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 5, 2024
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chaunceyjiang for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot
Copy link
Collaborator

Welcome @anujagrawal699! It looks like this is your first PR to karmada-io/karmada 🎉

@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 5, 2024
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.37%. Comparing base (8b4e006) to head (a02a1a6).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5301      +/-   ##
==========================================
+ Coverage   28.36%   28.37%   +0.01%     
==========================================
  Files         632      632              
  Lines       43774    43774              
==========================================
+ Hits        12416    12421       +5     
+ Misses      30458    30452       -6     
- Partials      900      901       +1     
Flag Coverage Δ
unittests 28.37% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@anujagrawal699
Copy link
Contributor Author

@XiShanYongYe-Chang I also added a comment for future reference if more types are registered.

@XiShanYongYe-Chang
Copy link
Member

Thanks for your contribution @anujagrawal699
/assign

@anujagrawal699
Copy link
Contributor Author

@XiShanYongYe-Chang It would be very helpful if you could check this
https://cloud-native.slack.com/archives/C02MUF8QXUN/p1722930839487819

@XiShanYongYe-Chang
Copy link
Member

@XiShanYongYe-Chang It would be very helpful if you could check this https://cloud-native.slack.com/archives/C02MUF8QXUN/p1722930839487819

Hi @anujagrawal699 the link is broken.

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Hi @anujagrawal699 Thanks for your contribution.
Although the current test content improves the file coverage, the test cases have no real meaning. In addition, the maintenance cost is increased. Therefore, I do not recommend incorporating this test.

You can find some other files to add some cases to maintain the code logic.

@anujagrawal699
Copy link
Contributor Author

Okay, Thanks. Should i close this PR?

@XiShanYongYe-Chang
Copy link
Member

Okay, Thanks. Should i close this PR?

Yes, let's close this pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants