Use uniquePermutations(ofCount:) in lazy split test.
#117
Conversation
This file contains 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
Concise! Eliminates the big array literals. Because we're now passing a huge variety of patterns to the same invocation of `Validator.validate()`, modified `Validator` to compute an interesting `maxSplits` for each tested case if none is provided at initialization.
|
@swift-ci Please test |
|
I think we should cut down on the time these tests take. The tests have already verified (to the extent they can) the correctness of the existing implementation. Going forward the tests will serve to catch regressions. But I think we will rarely change the implementation of lazy split, so 12 seconds (and even 2 seconds!) seems egregious. If there are specific interesting cases, like the ones of length 10 you call out, maybe we should hardcode those in addition to an exhaustive coverage of shorter sequences, say of length 0-6? |
I'll investigate doing something like what you suggest. Thanks for taking a look! |
kocsma
reviewed
Jul 31, 2021
| where T.Element == C.Element { | ||
| // Default max splits, omitting empty sequences | ||
| var testSplitCountOmittingEmpties: Int | ||
| switch separator { | ||
| case let .element(element): | ||
| let expected = s.split(separator: element).map { Array($0) } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Concise! Eliminates the big, unwieldy array literals.
Expanded test coverage...
Currently testing unique permutations of all subsets of length 0 through 10 of the sequence
||||||||||EEEEEEEEEE(|: separator;E: non-separator). See the comment abovetestAllLength0Through10()for the reasoning. That's quite a bit more coverage than the previous revision, which exhaustively tested only sequences of length 0 through 4, plus the unique permutations of select sequences of lengths 5 through 9....at a cost
Testing 2047 unique sequences has a cost of 3x longer test runtime. The lazy split tests alone now take almost 8 seconds on my machine. Total swift-algorithms test runtime is around 12 seconds, compared to 4 previously. If that's unacceptable, reducing the coverage to all possible patterns of lengths 0-9 reduces the lazy split test runtime to less than 2 seconds, and is still more extensive than the previous revision.
I can't make an argument that it's essential to cover all sequences up through length 10. I like that length 10 allows for certain interesting test cases like
||EE||EE||(multiple adjacent separators at beginning, middle and end, sandwiching subsequences of multiple elements), andE||EE||E||(adding an element to the beginning of a pattern quite similar to the previous one).It's more that
uniquePermutations(ofCount:)makes it so easy to add that much coverage, one might say, "why not?" To which another might reply, "because it takes too long to run." And I'm fine with either argument.Enhanced validation
Because we're now passing a great variety of different patterns to the same invocation of
Validator.validate(),Validatorhas been modified to compute an interestingmaxSplitsfor each tested case if none is provided at initialization.Checklist