[BatchMode] Fix non-portable test that relies on std::shuffle implementation. #15191
Conversation
|
@swift-ci please smoke test |
|
@davezarzycki I'd appreciate if you would be willing run this variant through to confirm that it works for you; I think it's not got any order-dependence in it anymore! |
|
I've downloaded the new test file and it appears to fix the test suite. |
|
Great, thanks! |
|
Hm, I guess we no longer test that it's actually shuffling, but since the shuffling itself is only for testing purposes maybe that's okay? |
|
@jrose-apple yeah, we're still testing the same sign-of-shuffle: the The thing it accidentally got coupled to was actually a part of the exactly-matched input that didn't look shuffled, it just happened to have batches 0, 1, 2, and 3 in order as a subsequence of the batches added-to on libcxx. The shuffled-parts were already regexp wildcard'ed out. |
|
Fascinating story. After reading it, but not deeply understanding it, I'm wondering if we need one combinatorial test? Or do we have one already?
… On Mar 12, 2018, at 8:38 PM, Graydon Hoare ***@***.***> wrote:
@jrose-apple <https://github.com/jrose-apple> yeah, we're still testing the same sign-of-shuffle: the SEED1-NOT: and SEED2-NOT: lines check that the files aren't added in 1, 2, 3 order.
The thing it accidentally got coupled to was actually a part of the exactly-matched input that didn't look shuffled, it just happened to have batches 0, 1, 2, and 3 in order as a subsequence of the batches added-to on libcxx. The shuffled-parts were already regexp wildcard'ed out.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#15191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcJ0AagG0-KW2t4hM7iJIsfcWgeQ0ZCks5tdz8jgaJpZM4SnxOz>.
|
|
Or maybe we need to run the test in Buffalo?
https://en.wikipedia.org/wiki/Shuffle_Off_to_Buffalo
… On Mar 12, 2018, at 8:38 PM, Graydon Hoare ***@***.***> wrote:
@jrose-apple <https://github.com/jrose-apple> yeah, we're still testing the same sign-of-shuffle: the SEED1-NOT: and SEED2-NOT: lines check that the files aren't added in 1, 2, 3 order.
The thing it accidentally got coupled to was actually a part of the exactly-matched input that didn't look shuffled, it just happened to have batches 0, 1, 2, and 3 in order as a subsequence of the batches added-to on libcxx. The shuffled-parts were already regexp wildcard'ed out.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#15191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcJ0AagG0-KW2t4hM7iJIsfcWgeQ0ZCks5tdz8jgaJpZM4SnxOz>.
|
|
@davidungar not sure what you mean by a combinatorial test. The test that broke here is one that runs the compiler 3 times, shuffling the partition in the latter 2 executions with different seeds. The test contains checks (intentionally) that the latter two runs do shuffle the partition: that the resulting shuffled batches do not contain the unshuffled 0-seed partition "file-1.swift, file-2.swift, file-3.swift". The test also contained checks (accidentally) that the resulting shuffled batches added files to their batches in a specific order (or, well, an order that had a specific subsequence). This was a mistake, over-specifying behaviour to that of a specific implementation of std::shuffle, which I have now removed. |
|
I was wondering if we would profit from a test that tried every possible partition? When I read "seed" I assumed the test was doing quasi-random shuffling.
That's what I was thinking, sorry for being unclear.
… On Mar 13, 2018, at 9:49 AM, Graydon Hoare ***@***.***> wrote:
@davidungar <https://github.com/davidungar> not sure what you mean by a combinatorial test. The test that broke here is one that runs the compiler 3 times, shuffling the partition in the latter 2 executions with different seeds.
The test contains checks (intentionally) that the latter two runs do shuffle the partition: that the resulting shuffled batches do not contain the unshuffled 0-seed partition "file-1.swift, file-2.swift, file-3.swift".
The test also contained checks (accidentally) that the resulting shuffled batches added files to their batches in a specific order (or, well, an order that had a specific subsequence). This was a mistake, over-specifying behaviour to that of a specific implementation of std::shuffle, which I have now removed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#15191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcJ0MyBSvydf9hxo49Qv7BjJsiX-0s8ks5td_iHgaJpZM4SnxOz>.
|
A sad little saga of random shuffles: in #15091 I changed the algorithm that the test-only
-driver-batch-seedmode uses to randomly assign files to batches, to make that algorithm preserve the invariant that batches are a subsequence of the top-level input sequence.The change made the driver shuffle the batch-selection order and not-shuffle (as it previously did) the file-assignment order. In so doing, I broke one of the existing tests that was supposed to be insensitive to randomization. The test tried to be randomization-insensitive by being insensitive to file-assignment order, but it accidentally remained sensitive to batch-selection order.
As it happens, this didn't break on my workstation because the sequence of batches wired-in to the test was a subsequence of the sequence of batches assigned by
std::shufflewhen running on libcxx. But only on a libcxx implementation. It turns out thatstd::shuffleisn't especially strictly specified, and libstdc++ doesn't use the same shuffling algorithm, so the test breaks on Fedora.