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

ref(derived_code_mappings): Pass frame filename to find_roots #85931

Merged
merged 5 commits into from
Feb 26, 2025

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented Feb 26, 2025

This change is in preparation for a follow-up change.

This change is in preparation of a follow-up change.
@armenzg armenzg self-assigned this Feb 26, 2025
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 26, 2025
@@ -119,7 +119,7 @@ def post(self, request: Request, project) -> Response:

branch = installation.extract_branch_from_source_url(repo, source_url)
source_path = installation.extract_source_path_from_source_url(repo, source_url)
stack_root, source_root = find_roots(stack_path, source_path)
stack_root, source_root = find_roots(FrameFilename({"filename": stack_path}), source_path)
Copy link
Member Author

Choose a reason for hiding this comment

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

A FrameFilename has more information than stack_path, thus, I can use it within find_roots.

@@ -86,7 +90,7 @@ def __init__(self, frame: Mapping[str, Any]) -> None:
self.full_path = frame_file_path
self.extension = get_extension(frame_file_path)
if not self.extension:
raise UnsupportedFrameFilename("It needs an extension.")
raise NeedsExtension("It needs an extension.")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an orthogonal change and it helps to differentiate from this code above:
image

"Unexpected format for stack_path or source_path",
extra={"stack_path": stack_path, "source_path": source_path},
)
logger.warning("Unexpected format for stack_path or source_path", extra=extra)
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing it to warning so I can inspect the errors in Sentry.

@@ -303,52 +301,76 @@ def test_list_file_matches_multiple(self) -> None:
assert matches == expected_matches

def test_find_roots_starts_with_period_slash(self) -> None:
stacktrace_root, source_path = find_roots("./app/", "static/app/")
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why I originally wrote the tests without a filename at the end of the path. It does not make sense to me.

From looking at the calls, it is obvious that two full paths are used rather than base dirs:

stack_path = frame_filename.raw_path
source_path = file
try:
stack_root, source_root = find_roots(stack_path, source_path)

@armenzg armenzg marked this pull request as ready for review February 26, 2025 13:49
@armenzg armenzg requested a review from a team as a code owner February 26, 2025 13:49
Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...try/issues/auto_source_code_config/code_mapping.py 75.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #85931       +/-   ##
===========================================
+ Coverage   60.53%   87.89%   +27.36%     
===========================================
  Files        9677     9686        +9     
  Lines      548684   549251      +567     
  Branches    21312    21312               
===========================================
+ Hits       332130   482769   +150639     
+ Misses     216238    66166   -150072     
  Partials      316      316               

@armenzg armenzg merged commit 99d98f4 into master Feb 26, 2025
49 checks passed
@armenzg armenzg deleted the ref/frame_filename/derive_code_mapping/armenzg branch February 26, 2025 16:24
Copy link

sentry-io bot commented Feb 26, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ NeedsExtension: It needs an extension. sentry.tasks.auto_source_code_config View Issue
  • ‼️ UnsupportedFrameFilename: This path is not supported. /api/0/projects/{organization_id_or_slug}/{proj... View Issue
  • ‼️ UnexpectedPathException: Could not find common root from paths /api/0/projects/{organization_id_or_slug}/{proj... View Issue

Did you find this useful? React with a 👍 or 👎

ameliahsu pushed a commit that referenced this pull request Mar 5, 2025
This change is in preparation for a follow-up change.
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants