Skip to content

Simplify failureBreakpoint() and add a unit test. #557

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Simplify failureBreakpoint() and add a unit test. #557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions Sources/Testing/Issues/Issue+Recording.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,9 @@ extension Issue {

// MARK: - Debugging failures

/// A unique value used by ``failureBreakpoint()``.
@usableFromInline nonisolated(unsafe) var failureBreakpointValue = 0

/// A function called by the testing library when a failure occurs.
///
/// Whenever a test failure (specifically, a non-known ``Issue``) is recorded,
Expand Down Expand Up @@ -265,11 +268,7 @@ func failureBreakpoint() {
// behavior can be disabled by passing the `-no_deduplicate` flag described in
// ld(1), but that would disable it module-wide and sacrifice optimization
// opportunities elsewhere. Instead, this function performs a trivial
// function call, passing it a sufficiently unique value to avoid
// de-duplication.
struct NoOp {
nonisolated(unsafe) static var ignored: Int = 0
static func perform(_: inout Int) {}
}
NoOp.perform(&NoOp.ignored)
// operation on a usable-from-inline value, which the compiler must assume
// cannot be optimized away.
failureBreakpointValue = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to enable the Thread Sanitizer and run our test suite, would it flag this as a data race? (Despite always assigning the same value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, but if it did I would expect the original implementation would have the same issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be surprised if both of them did not report a TSan race. But you're saying you tried and they did not? 🤔

Either way, this PR seems fine. I just really want to avoid transforming this into a global incrementing counter in the future, since then it would require a lock and would increase the overhead of every test failure.

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 tried and didn't see any race reported even when I bombarded it with a whole bunch of concurrent detached tasks. So if TSan is watching that address, it needs more coffee.

But that all said, we can switch to an atomic store in the future if we did want a counter. No lock needed.

}
7 changes: 7 additions & 0 deletions Tests/TestingTests/MiscellaneousTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,4 +525,11 @@ struct MiscellaneousTests {
#expect(mappedTests.count == tests.count)
#expect(mappedTests.values.allSatisfy { tests.contains($0) })
}

@Test("failureBreakpoint() call")
func failureBreakpointCall() {
failureBreakpointValue = 0
failureBreakpoint()
#expect(failureBreakpointValue == 1)
}
}