[Do not merge] Testing codecov AI flows #1250
Conversation
✅ Sentry found no issues in your recent changes ✅ |
|
@codecov-ai-reviewer test |
|
On it! Codecov is generating unit tests for this PR. |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1250 +/- ##
==========================================
- Coverage 96.31% 96.11% -0.21%
==========================================
Files 492 492
Lines 16846 16891 +45
==========================================
+ Hits 16226 16235 +9
- Misses 620 656 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
|
@codecov-ai-reviewer test |
|
On it! Codecov is generating unit tests for this PR. |
|
@codecov-ai-reviewer test |
|
On it! Codecov is generating unit tests for this PR. |
|
@codecov-ai-reviewer test |
|
On it! Codecov is generating unit tests for this PR. |
|
Sentry has determined that unit tests are not necessary for this PR. |
|
@codecov-ai-reviewer review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
![codecov-ai[bot]](https://avatars.githubusercontent.com/in/797565?s=60&v=4){kind=small}
|
|
||
| def get_bundle_size(self) -> int | None: | ||
| try: | ||
| repo = self.repo | ||
| except Http404: | ||
| log.warning("Repo not found", extra=dict(repo=self.kwargs.get("repo_name"))) | ||
| return None | ||
|
|
||
| if repo.private and repo.image_token != self.request.query_params.get("token"): | ||
| log.warning( | ||
| "Token provided does not match repo's image token", | ||
| extra=dict(repo=repo), | ||
| ) | ||
| return None | ||
|
|
||
| branch_name = self.kwargs.get("branch") or repo.branch | ||
| branch = Branch.objects.filter( | ||
| name=branch_name, repository_id=repo.repoid | ||
| ).first() | ||
|
|
||
| if branch is None: | ||
| log.warning( | ||
| "Branch not found", extra=dict(branch_name=branch_name, repo=repo) | ||
| ) | ||
| return None | ||
|
|
||
| try: | ||
| commit: Commit = repo.commits.filter(commitid=branch.head).first() | ||
| except ObjectDoesNotExist: | ||
| log.warning("Commit not found", extra=dict(commit=branch.head)) | ||
| return None | ||
|
|
||
| commit_bundles = load_report(commit) | ||
|
|
||
| if commit_bundles is None: | ||
| log.warning( | ||
| "Bundle analysis report not found for commit", | ||
| extra=dict(commit=branch.head), | ||
| ) | ||
| return None | ||
|
|
||
| bundle_name = str(self.kwargs.get("bundle")) | ||
| bundle = commit_bundles.bundle_report(bundle_name) | ||
|
|
||
| if bundle is None: | ||
| log.warning( | ||
| "Bundle with provided name not found for commit", | ||
| extra=dict(commit=branch.head), | ||
| ) | ||
| return None |
There was a problem hiding this comment.
The get_bundle_size method is quite long and handles multiple responsibilities (permission checking, repo validation, branch lookup, commit lookup, bundle loading). Consider breaking this down into smaller, more focused methods to improve readability and maintainability. This would also make it easier to test individual pieces of functionality.
| filename = "bundle-badge" | ||
|
|
||
| def get_object(self, request, *args, **kwargs): | ||
| # Validate precision query param | ||
| precision = self.request.query_params.get("precision", "2") | ||
| if precision not in self.precisions: | ||
| raise NotFound("Bundle size precision should be one of [ 0 || 1 || 2 ]") |
There was a problem hiding this comment.
The error handling for precision validation could be moved to a separate validation method, following the Single Responsibility Principle. Also, consider using Django's built-in validators or form validation for this.
| re_path( | ||
| "branch/(?P<branch>.+)/(graph|graphs)/bundle/(?P<bundle>.+)/badge.(?P<ext>[^/]+)", | ||
| BundleBadgeHandler.as_view(), | ||
| name="branch-bundle-badge", | ||
| ), | ||
| re_path( | ||
| "(graph|graphs)/bundle/(?P<bundle>.+)/badge.(?P<ext>[^/]+)", | ||
| BundleBadgeHandler.as_view(), | ||
| name="default-bundle-badge", |
There was a problem hiding this comment.
The URL patterns for bundle badges are similar to existing patterns. Consider using Django's path() instead of re_path() for improved readability and maintainability. Also, the regex patterns could be simplified and constants could be used for repeated patterns.
|
|
||
| def get_bundle_size(self) -> int | None: | ||
| try: | ||
| repo = self.repo | ||
| except Http404: | ||
| log.warning("Repo not found", extra=dict(repo=self.kwargs.get("repo_name"))) | ||
| return None | ||
|
|
||
| if repo.private and repo.image_token != self.request.query_params.get("token"): | ||
| log.warning( | ||
| "Token provided does not match repo's image token", | ||
| extra=dict(repo=repo), | ||
| ) | ||
| return None | ||
|
|
||
| branch_name = self.kwargs.get("branch") or repo.branch | ||
| branch = Branch.objects.filter( | ||
| name=branch_name, repository_id=repo.repoid | ||
| ).first() | ||
|
|
||
| if branch is None: | ||
| log.warning( | ||
| "Branch not found", extra=dict(branch_name=branch_name, repo=repo) | ||
| ) | ||
| return None | ||
|
|
||
| try: | ||
| commit: Commit = repo.commits.filter(commitid=branch.head).first() | ||
| except ObjectDoesNotExist: | ||
| log.warning("Commit not found", extra=dict(commit=branch.head)) | ||
| return None | ||
|
|
||
| commit_bundles = load_report(commit) | ||
|
|
||
| if commit_bundles is None: | ||
| log.warning( | ||
| "Bundle analysis report not found for commit", | ||
| extra=dict(commit=branch.head), | ||
| ) | ||
| return None | ||
|
|
||
| bundle_name = str(self.kwargs.get("bundle")) | ||
| bundle = commit_bundles.bundle_report(bundle_name) | ||
|
|
||
| if bundle is None: | ||
| log.warning( | ||
| "Bundle with provided name not found for commit", | ||
| extra=dict(commit=branch.head), | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Consider adding caching for bundle size calculations to improve performance, especially for frequently accessed badges. This could be implemented using Django's cache framework.
Purpose/Motivation
What is the feature? Why is this being done?
Links to relevant tickets
What does this PR do?
Include a brief description of the changes in this PR. Bullet points are your friend.
Notes to Reviewer
Anything to note to the team? Any tips on how to review, or where to start?
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.