-
Notifications
You must be signed in to change notification settings - Fork 108
Represent non-encodable test argument values in Test.Case.ID #1000
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
Changes from 5 commits
616dd87
ae2d351
0170819
9c5d989
eb6f4ec
d123990
79caae8
d710f46
2a0827b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,7 +62,7 @@ extension Test.Case { | |
// A beautiful hack to give us the right number of cases: iterate over a | ||
// collection containing a single Void value. | ||
self.init(sequence: CollectionOfOne(())) { _ in | ||
Test.Case(arguments: [], body: testFunction) | ||
Test.Case(body: testFunction) | ||
} | ||
} | ||
|
||
|
@@ -257,7 +257,32 @@ extension Test.Case { | |
|
||
extension Test.Case.Generator: Sequence { | ||
func makeIterator() -> some IteratorProtocol<Test.Case> { | ||
_sequence.lazy.map(_mapElement).makeIterator() | ||
let state = ( | ||
iterator: _sequence.makeIterator(), | ||
testCaseIDs: [Test.Case.ID: Int](minimumCapacity: underestimatedCount) | ||
) | ||
|
||
return sequence(state: state) { state in | ||
guard let element = state.iterator.next() else { | ||
return nil | ||
} | ||
|
||
var testCase = _mapElement(element) | ||
|
||
if testCase.isParameterized { | ||
// Store the original, unmodified test case ID. We're about to modify a | ||
// property which affects it, and we want to update state based on the | ||
// original one. | ||
let testCaseID = testCase.id | ||
|
||
// Ensure test cases with identical IDs each have a unique discriminator. | ||
let discriminator = state.testCaseIDs[testCaseID, default: 0] | ||
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. What's the benefit of maintaining a dictionary here? What if the discriminator were just the 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 goal is different than you may be thinking: I'm not trying to assign each test case the next-highest integer as the discriminator, which would be equivalent to the "enumeration index" of the collection; instead I'm trying to locate the test cases for each parameterized test function which have identical arguments, and for each such subset, assign each of those test cases a unique discriminator. Consider this example: @Test(arguments: [9, 9, 8, 7]
func example(_: Int) {} This will have 4 test cases, and with this PR's changes, they should be (in pseudo-code):
The end goal is to be able to detect duplicates of the same set of arguments. If we used 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. I see—is there something we can do that's less heavyweight than a hash table? 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. Not sure. The use of a dictionary doesn't seem that heavyweight to me in context. This isn't now causing values to be encoded where they wouldn't otherwise be, if that's the concern. One small optimization I just realized I could make, though, is to specify a capacity for the dictionary based on the generator's 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. A concern I have is that this is going to scale at least linearly with the number of test cases in a test. As of right now, we try to avoid keeping information about all test cases in memory during a run. If the inputs are an artisanally crafted array of values, then that's one thing. But if there's a dynamically generated sequence of inputs that's particularly long, we're now going to pay a memory cost to track all of them. 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 dictionary only stores their IDs, not the test cases themselves, and I don't expect that to be onerous. I definitely share your view that we should not be holding on to test case instances themselves for the duration of the test run, or even for the lifetime of a We already "accumulate" the IDs of |
||
testCase.discriminator = discriminator | ||
state.testCaseIDs[testCaseID] = discriminator + 1 | ||
} | ||
|
||
return testCase | ||
} | ||
} | ||
|
||
var underestimatedCount: Int { | ||
|
Uh oh!
There was an error while loading. Please reload this page.