Use `uniquePermutations(ofCount:)` in lazy split test.

toddthomas commented 3 years ago

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 above testAllLength0Through10() 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), and E||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(), Validator has been modified to compute an interesting maxSplits for each tested case if none is provided at initialization.

Checklist

[x] I've added at least one test that validates that my change is working, if appropriate
[x] I've followed the code style of the rest of the project
[x] I've read the Contribution Guidelines
[x] I've updated the documentation if necessary

natecook1000 commented 3 years ago

@swift-ci Please test

kylemacomber commented 3 years ago

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?

toddthomas commented 3 years ago

I think we should cut down on the time these tests take.

I'll investigate doing something like what you suggest. Thanks for taking a look!

apple / swift-algorithms