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

Add additional test coverage for common functions in GitHub provider #4648

Conversation

gajananan
Copy link
Contributor

@gajananan gajananan commented Oct 5, 2024

Summary

Currently common functions in GitHub provider lacks test coverage when converting github repos to minder repos.
This commit adds test coverage for ConvertRepositories for github.

Ref #4380

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

Currently common functions in GitHub provider lacks test coverage when converting github repos to minder repos.

This commit adds test coverage for ConvertRepositories for github

Signed-off-by: Kugamoorthy Gajananan <[email protected]>
@coveralls
Copy link

coveralls commented Oct 5, 2024

Coverage Status

coverage: 53.361% (+0.1%) from 53.264%
when pulling dfd73e8 on gajananan:improve-test-coverage-proividers-gh-common
into 49f192a on stacklok:main.

Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @gajananan!

The code changes look good, there are just a few cleanup items we need before merging this.
Could you run make lint on you local machine before committing? This will show the lint errors.
Could you change the PR description from Fixes #4380 to Ref #4380? If we have the Fixes keyword then it will close the associated issue, which we don't want in this case.

Currently helper functions in Util lacks test coverage.

This commit
- fixes the lint errors and handles errors.
- Adds test coverage for LoadCredentials function

Signed-off-by: Kugamoorthy Gajananan <[email protected]>
@gajananan
Copy link
Contributor Author

gajananan commented Oct 7, 2024

Thanks for the PR @gajananan!

The code changes look good, there are just a few cleanup items we need before merging this. Could you run make lint on you local machine before committing? This will show the lint errors. Could you change the PR description from Fixes #4380 to Ref #4380? If we have the Fixes keyword then it will close the associated issue, which we don't want in this case.

Thank you. @eleftherias. I managed fix the lint config on my machine and run the make lint properly. it looks fine locally (no lint errors) in my machine now. However, lint seems to be failing in CI.

@eleftherias
Copy link
Contributor

Thanks for the PR @gajananan!
The code changes look good, there are just a few cleanup items we need before merging this. Could you run make lint on you local machine before committing? This will show the lint errors. Could you change the PR description from Fixes #4380 to Ref #4380? If we have the Fixes keyword then it will close the associated issue, which we don't want in this case.

Thank you. @eleftherias. I managed fix the lint config on my machine and run the make lint properly. it looks fine locally (no lint errors) in my machine now. However, lint seems to be failing in CI.

It could be a mismatched version of the lint tool. Could you try running make bootstrap (to install the required version) and then make lint again?

Currently common functions in GitHub provider lacks test coverage when converting github repos to minder repos.

This commit fixes the lint errors after fixing lint version.

Signed-off-by: Kugamoorthy Gajananan <[email protected]>
@gajananan
Copy link
Contributor Author

Thanks for the PR @gajananan!
The code changes look good, there are just a few cleanup items we need before merging this. Could you run make lint on you local machine before committing? This will show the lint errors. Could you change the PR description from Fixes #4380 to Ref #4380? If we have the Fixes keyword then it will close the associated issue, which we don't want in this case.

Thank you. @eleftherias. I managed fix the lint config on my machine and run the make lint properly. it looks fine locally (no lint errors) in my machine now. However, lint seems to be failing in CI.

It could be a mismatched version of the lint tool. Could you try running make bootstrap (to install the required version) and then make lint again?

Thank you, @eleftherias. I was not aware I was using an old version of lint. It is now fixed.

Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @gajananan!

@eleftherias eleftherias merged commit 76e9f43 into mindersec:main Oct 7, 2024
19 checks passed
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.

3 participants