-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
616dd87
Represent non-encodable test argument values in Test.Case.ID
stmontgomery ae2d351
Improve the modeling of test cases (and their IDs) for non-parameteri…
stmontgomery 0170819
Revert changes to Test.Case.Argument.Snapshot which aren't necessary
stmontgomery 9c5d989
Incorporate various review feedback
stmontgomery eb6f4ec
Remove unnecessary conformances for Test.Case
stmontgomery d123990
Use a for loop instead of reduce
stmontgomery 79caae8
Switch from some to any types for Codable implementations
stmontgomery d710f46
Simplify everything by storing `isStable` on Test.Case.ID instead of …
stmontgomery 2a0827b
Refine Codable implementation of Test.Case.ID to maintain behavior wi…
stmontgomery 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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
What's the benefit of maintaining a dictionary here? What if the discriminator were just the
enumerated()
index of the case? Yes, we'd end up skipping over some discriminators, but so what—they'll still be unique in this context, and it'll be much cheaper to compute and store them. [P2]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.
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:
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
enumerated()
on the original collection that would be harder for clients to detect duplicates, and for the testing library itself to detect and report warnings for them (which I intend to do in a follow-on PR). I also worry that would tempt clients to abuse thediscriminator
value and interpret it as an index of the original collection, and that's explicitly not what I'm intending—particularly because the order may differ between successive runs (for example for hashed collections).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.
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 comment
The 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
underestimatedCount
.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.
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 comment
The 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
Test.Case.Generator
.We already "accumulate" the IDs of
Test
instances in places likeEvent.HumanReadableOutputRecorder
to keep track of events per-test. I imagine in the not-too-distant future we'll want to extend that pattern to test cases too, so that (for example) when running in verbose mode, our console output could log when test cases finish and how many failures each one had. That topic came up in #943, and was only partly solved by #972. One of my motivations for this PR is to allow us to properly implement that. I've been meaning to file a new issue to track that, so I just filed #1021.