Skip to content

Require explicit backtrace and source location when creating and recording issues #688

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

stmontgomery
Copy link
Contributor

@stmontgomery stmontgomery commented Sep 12, 2024

This refactors some of the logic for constructing SourceContext and Issue instances to require explicit arguments in more places.

Motivation:

We can provide clearer and more relevant details about recorded issues by ensuring that their backtrace and/or source location are accurate. In general, whenever collecting a backtrace, it's better to elide unhelpful frames from the testing library itself so that frames from user code are as close to the top of the call stack as possible. Similarly, whenever an explicit source location is known, it's best to propagate that in any recorded issues which originate at that location.

Currently, the SourceContext type has default parameter values for both its backtrace and sourceLocation properties. Having default values for these parameters means that callers elsewhere in the codebase may accidentally not specify a value, thereby not propagating useful information. Or, in the case of backtraces, they may rely on the default value which results in additional, irrelevant frames in the call stack (representing thunks inserted by the compiler for the default value). The initializer for Issue also includes a default value for one of its parameters.

Modifications:

  • Change SourceContext.init(...) to remove the default values for both of its parameters: backtrace: and sourceLocation:.
  • Change Issue.init(...) to no longer have default values for any of its parameters.
  • Remove the non-public, static Issue.record(...) functions, and change callers to rely on the instance record(configuration:) method. This consolidates the usage pattern across the Testing module for creating an Issue and then recording it.
  • Change SkipInfo.init(...) to remove the default value for its sourceContext: parameter.
  • Add a convenience init(for:sourceLocation:) which accepts an Error and correctly creates a SourceContext for it.
  • Add deprecated overloads of all SPIs which were modified to remove default parameter values.
  • Add convenience overloads to TestingAdditions.swift which preserve default parameter values, to avoid unnecessary boilerplate in test-only code where it's safe to rely on such default values.

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

@grynspan
Copy link
Contributor

This is internal code, so it doesn't make a huge difference, but… the intent behind not having a default value is to force us callers to always spell it out, lest we accidentally forget to forward comments from our own callers.

Is this fixing a known issue or just housekeeping?

Copy link
Contributor

@grynspan grynspan left a comment

Choose a reason for hiding this comment

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

This change would introduce regressions in our backtrace quality and could cause us to record issues with no source context at all.

@grynspan grynspan added this to the Swift 6.1 milestone Sep 12, 2024
@stmontgomery stmontgomery added the issue-handling Related to Issue handling within the testing library label Sep 12, 2024
@stmontgomery stmontgomery changed the title Default 'comments:' to an empty array when creating an Issue Require explicit backtrace and source location when creating and recording issues Sep 12, 2024
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

@stmontgomery stmontgomery merged commit fc35539 into swiftlang:main Sep 13, 2024
3 checks passed
@stmontgomery stmontgomery deleted the issue-default-comments branch September 13, 2024 15:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request issue-handling Related to Issue handling within the testing library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants