-
Notifications
You must be signed in to change notification settings - Fork 112
Refactor test content discovery to produce a sequence. #914
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e63d194
Refactor test content discovery to produce a sequence.
grynspan a1ac940
Where did ~Copyable go?
grynspan a0134eb
Incorporate feedback, work around compiler crash
grynspan 93f7549
Crash was not where I thought it was maybe?
grynspan 68f252d
Suppress some warnings in the test target
grynspan ab93de9
Try to work around the crash another way
grynspan b8f5cbb
Don't break my own tests
grynspan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,59 +22,51 @@ extension Test: TestContent { | |
/// The order of values in this sequence is unspecified. | ||
static var all: some Sequence<Self> { | ||
get async { | ||
var generators = [@Sendable () async -> [Self]]() | ||
// The result is a set rather than an array to deduplicate tests that were | ||
// generated multiple times (e.g. from multiple discovery modes or from | ||
// defective test records.) | ||
var result = Set<Self>() | ||
|
||
// Figure out which discovery mechanism to use. By default, we'll use both | ||
// the legacy and new mechanisms, but we can set an environment variable | ||
// to explicitly select one or the other. When we remove legacy support, | ||
// we can also remove this enumeration and environment variable check. | ||
enum DiscoveryMode { | ||
case tryBoth | ||
case newOnly | ||
case legacyOnly | ||
} | ||
let discoveryMode: DiscoveryMode = switch Environment.flag(named: "SWT_USE_LEGACY_TEST_DISCOVERY") { | ||
let (useNewMode, useLegacyMode) = switch Environment.flag(named: "SWT_USE_LEGACY_TEST_DISCOVERY") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change makes it easier to disable this code in #880. |
||
case .none: | ||
.tryBoth | ||
(true, true) | ||
case .some(true): | ||
.legacyOnly | ||
(false, true) | ||
case .some(false): | ||
.newOnly | ||
(true, false) | ||
} | ||
|
||
// Walk all test content and gather generator functions. Note we don't | ||
// actually call the generators yet because enumerating test content may | ||
// involve holding some internal lock such as the ones in libobjc or | ||
// dl_iterate_phdr(), and we don't want to accidentally deadlock if the | ||
// user code we call ends up loading another image. | ||
if discoveryMode != .legacyOnly { | ||
enumerateTestContent { imageAddress, generator, _, _ in | ||
generators.append { @Sendable in | ||
await [generator()] | ||
// Walk all test content and gather generator functions, then call them in | ||
// a task group and collate their results. | ||
if useNewMode { | ||
let generators = Self.discover().lazy.compactMap { $0.load() } | ||
await withTaskGroup(of: Self.self) { taskGroup in | ||
for generator in generators { | ||
taskGroup.addTask(operation: generator) | ||
} | ||
result = await taskGroup.reduce(into: result) { $0.insert($1) } | ||
} | ||
} | ||
|
||
if discoveryMode != .newOnly && generators.isEmpty { | ||
generators += types(withNamesContaining: testContainerTypeNameMagic).lazy | ||
// Perform legacy test discovery if needed. | ||
if useLegacyMode && result.isEmpty { | ||
let types = types(withNamesContaining: testContainerTypeNameMagic).lazy | ||
.compactMap { $0 as? any __TestContainer.Type } | ||
.map { type in | ||
{ @Sendable in await type.__tests } | ||
} | ||
} | ||
|
||
// *Now* we call all the generators and return their results. | ||
// Reduce into a set rather than an array to deduplicate tests that were | ||
// generated multiple times (e.g. from multiple discovery modes or from | ||
// defective test records.) | ||
return await withTaskGroup(of: [Self].self) { taskGroup in | ||
for generator in generators { | ||
taskGroup.addTask { | ||
await generator() | ||
await withTaskGroup(of: [Self].self) { taskGroup in | ||
for type in types { | ||
taskGroup.addTask { | ||
await type.__tests | ||
} | ||
} | ||
result = await taskGroup.reduce(into: result) { $0.formUnion($1) } | ||
} | ||
return await taskGroup.reduce(into: Set()) { $0.formUnion($1) } | ||
} | ||
|
||
return result | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -615,43 +615,45 @@ struct MiscellaneousTests { | |
0xABCD1234, | ||
0, | ||
{ outValue, hint in | ||
if let hint, hint.loadUnaligned(as: TestContentAccessorHint.self) != expectedHint { | ||
if let hint, hint.load(as: TestContentAccessorHint.self) != expectedHint { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hint is always well-aligned. |
||
return false | ||
} | ||
_ = outValue.initializeMemory(as: TestContentAccessorResult.self, to: expectedResult) | ||
return true | ||
}, | ||
UInt(UInt64(0x0204060801030507) & UInt64(UInt.max)), | ||
UInt(truncatingIfNeeded: UInt64(0x0204060801030507)), | ||
0 | ||
) | ||
} | ||
|
||
@Test func testDiscovery() async { | ||
await confirmation("Can find a single test record") { found in | ||
DiscoverableTestContent.enumerateTestContent { _, value, context, _ in | ||
if value == DiscoverableTestContent.expectedResult && context == DiscoverableTestContent.expectedContext { | ||
found() | ||
} | ||
} | ||
} | ||
|
||
await confirmation("Can find a test record with matching hint") { found in | ||
// Check the type of the test record sequence (it should be lazy.) | ||
let allRecords = DiscoverableTestContent.discover() | ||
grynspan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#expect(allRecords is any LazySequenceProtocol) | ||
#expect(!(allRecords is [TestContentRecord<DiscoverableTestContent>])) | ||
|
||
// It should have exactly one matching record (because we only emitted one.) | ||
#expect(Array(allRecords).count == 1) | ||
|
||
// Can find a single test record | ||
#expect(allRecords.contains { record in | ||
record.load() == DiscoverableTestContent.expectedResult | ||
&& record.context == DiscoverableTestContent.expectedContext | ||
}) | ||
|
||
// Can find a test record with matching hint | ||
#expect(allRecords.contains { record in | ||
let hint = DiscoverableTestContent.expectedHint | ||
DiscoverableTestContent.enumerateTestContent(withHint: hint) { _, value, context, _ in | ||
if value == DiscoverableTestContent.expectedResult && context == DiscoverableTestContent.expectedContext { | ||
found() | ||
} | ||
} | ||
} | ||
return record.load(withHint: hint) == DiscoverableTestContent.expectedResult | ||
&& record.context == DiscoverableTestContent.expectedContext | ||
}) | ||
|
||
await confirmation("Doesn't find a test record with a mismatched hint", expectedCount: 0) { found in | ||
// Doesn't find a test record with a mismatched hint | ||
#expect(!allRecords.contains { record in | ||
let hint = ~DiscoverableTestContent.expectedHint | ||
DiscoverableTestContent.enumerateTestContent(withHint: hint) { _, value, context, _ in | ||
if value == DiscoverableTestContent.expectedResult && context == DiscoverableTestContent.expectedContext { | ||
found() | ||
} | ||
} | ||
} | ||
return record.load(withHint: hint) == DiscoverableTestContent.expectedResult | ||
&& record.context == DiscoverableTestContent.expectedContext | ||
}) | ||
} | ||
#endif | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you talk about the choice of name? I might suggest a property instead of a function and naming it something like
allRecords
orallTestContentRecords
. At the call site,discover()
feels more like an imperative instruction rather than a call which returns a value to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.