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 [Coderabbit] PR Stats service and tests #10749

Merged
merged 14 commits into from
Dec 29, 2024

Conversation

aravindputrevu
Copy link
Contributor

@aravindputrevu aravindputrevu commented Dec 16, 2024

  • Implemented a new service CoderabbitStats to fetch and display pull request statistics from the CodeRabbit API.
  • Created a corresponding tester file to validate the service's functionality, including tests for valid repositories, repository not found, and server errors.
  • The service returns a badge with the number of PRs and appropriate error messages based on the API response.

This addition enhances the analysis capabilities of the application by integrating CodeRabbit statistics.

GitHub Issue - #10748

- Implemented a new service `CoderabbitStats` to fetch and display pull request statistics from the CodeRabbit API.
- Created a corresponding tester file to validate the service's functionality, including tests for valid repositories, repository not found, and server errors.
- The service returns a badge with the number of PRs and appropriate error messages based on the API response.

This addition enhances the analysis capabilities of the application by integrating CodeRabbit statistics.
Copy link
Contributor

github-actions bot commented Dec 16, 2024

Messages
📖 ✨ Thanks for your contribution to Shields, @aravindputrevu!

Generated by 🚫 dangerJS against 8470c88

@chris48s chris48s changed the title Add Coderabbit PR Stats service and tests Add [Coderabbit] PR Stats service and tests Dec 16, 2024
@chris48s chris48s added the service-badge New or updated service badge label Dec 16, 2024
Copy link
Member

@jNullj jNullj left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.
I noticed you did not add any live tests (making real API calls to external service) and only mocked responses.
Please give a quick read to this section of our docs about tests, as mentioned its best to include at least one live test so we know our service is still live and up to date with upstream.

Note that when we call our badge, we are allowing it to communicate with an external service without mocking the response. We write tests which interact with external services, which is unusual practice in unit testing. We do this because one of the purposes of service tests is to notify us if a badge has broken due to an upstream API change. For this reason it is important for at least one test to call the live API without mocking the interaction

Is there a specific reason to avoid using live tests? If no could you add at least one live test, Its common for our project to have all calls to live services in tests unless we test internal functionality like data transformation of API response.

- Updated the service to fetch and display the number of reviews from the CodeRabbit API, changing the schema and badge labels accordingly.
- Modified the tester file to reflect the new endpoint and expected responses, including regex for message validation.
- Enhanced error handling in the service to return more descriptive error messages for invalid repositories and server errors.

This change improves the accuracy of the statistics provided by the service, aligning it with the intended functionality of tracking reviews.
@aravindputrevu
Copy link
Contributor Author

@jNullj I have made relevant changes as per your request, requesting you to see if it fits the guidelines.

services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
- Updated the CodeRabbitStats service to include OpenAPI documentation and improved error handling for repository not found scenarios.
- Changed badge label from 'CodeRabbit' to 'CodeRabbit Reviews' for clarity.
- Modified the tester file to reflect the new badge format and error messages, ensuring consistency with the service updates.
- Adjusted regex patterns for message validation in tests.

These changes improve the usability and accuracy of the CodeRabbit statistics service.
@aravindputrevu
Copy link
Contributor Author

@chris48s I'm contributing a badge for the first time to shields.io.

Sorry for the trouble with obvious mistakes. I've read the contributor.md and tutorial.md, but I still missed a lot. Please re-review and let me know if things are out of place.

services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.tester.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.tester.js Outdated Show resolved Hide resolved
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Latest draft is looking pretty good. There's a few more bits on this latest revision, but we're getting there. Thanks

services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
Comment on lines 24 to 25
example: 'github, gitlab, bitbucket',
description: 'Version Control Provider (e.g., github)',
Copy link
Member

Choose a reason for hiding this comment

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

The example value should be a value that works, so "github" would be sensible value for example here. Basically if I copy and paste it into the builder, it should work.

We also don't need to double up examples in the description.

In the docs, if we want to communicate to the user that there is a closed set of valid values, we can give the user a dropdown to pick from. So if you look at something like the vcsType param on https://shields.io/badges/coveralls you get a dropdown for [github, bitbucket, gitlab].

Screenshot at 2024-12-23 18-12-35

Here's what that looks like in the code.

pattern: ':vcsType(github|bitbucket|gitlab)/:user/:repo+',

pathParam({
name: 'vcsType',
example: 'github',
schema: { type: 'string', enum: this.getEnum('vcsType') },
}),

Lets do the same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok for the time-being it showing only GH. As GH has most usage, I'll be happy to update in the next revision with the upcoming badges.

Editing the text to only show GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Are gitlab and bitbucket values that work today, or not?

If they are, lets make this

static route = {
  base: 'coderabbit',
  pattern: 'prs/:provider(github|bitbucket|gitlab)/:org/:repo',
}

in the route and

{
  name: 'provider',
  example: 'github',
  description: 'Version Control Provider',
  schema: { type: 'string', enum: this.getEnum('provider') },
}

in the Open API spec.

That will show it as

Screenshot at 2024-12-23 20-21-57

in the docs, and we're good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change as you mentioned. Thanks for the suggestion!

services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
aravindputrevu and others added 4 commits December 23, 2024 19:32
- Changed example section in CodeRabbitStats service from 'github, gitlab, bitbucket' to 'github' as per review comment.
- Updated error message for 404 response to 'provider or repo not found', to reflect the right code.
Copy link
Member

@jNullj jNullj left a comment

Choose a reason for hiding this comment

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

Looks good, i run it and it seems to work as intended.
I have 2 more comments, I think that other then this 2 I could approve the PR.
Take note that we usually wait until all reviewers involved with the PR approve before merging the PR.

services/coderabbit/coderabbit-stats.tester.js Outdated Show resolved Hide resolved
services/coderabbit/coderabbit-stats.service.js Outdated Show resolved Hide resolved
Copy link
Member

@jNullj jNullj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

🚀 Updated review app: https://pr-10749-badges-shields.fly.dev

@chris48s chris48s added this pull request to the merge queue Dec 29, 2024
Merged via the queue into badges:master with commit cb30902 Dec 29, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants