Adding validation tests for LazyFilterCollection and LazyMapCollection #2247
Conversation
|
The full test results can be found here (warning: large text file) https://gist.githubusercontent.com/austinzheng/273fe40566a02e249d54828915d10ba7/raw/cbf90ec4eaeddcde37c2e0ebad56d99a2b8e307a/seqerror.txt |
| @@ -60,6 +60,8 @@ extension LifetimeTracked : CustomStringConvertible { | |||
| } | |||
| } | |||
|
|
|||
| extension LifetimeTracked : Comparable { } | |||
There was a problem hiding this comment.
I thought Strideable implies Comparable.
There was a problem hiding this comment.
Oops. I'll fix this.
austinzheng
force-pushed
the
swift-3-indexing-model
branch
from
382a266 to
dd25a28
Compare
| // Test collection as random access collection using value types as elements. | ||
| CollectionTests.addRandomAccessCollectionTests ( | ||
| makeCollection: { (elements: [OpaqueValue<Int>]) in | ||
| elements.lazy.map(identity) |
There was a problem hiding this comment.
You should wrap elements into a minimal collection of appropriate kind. This will check for more issues.
There was a problem hiding this comment.
There are four kinds of lazy maps (sequence, forward collection, bidirectional, random access). You can get instances of each by calling the lazy map on a corresponding minimal collection or sequence. Could you add tests for all?
|
I looked through the test failures and I think you will see fewer of them once you wrap the lazy collections around minimal collections. |
|
Thanks for the review! I'll have an update shortly (hopefully). |
|
Thanks @austinzheng! Your PR would be a great addition to our test coverage! |
austinzheng
force-pushed
the
swift-3-indexing-model
branch
from
dd25a28 to
bcfe02a
Compare
|
Hello @gribozavr! I've updated the PR. The new failures are: Note that the tests that are failing are configurable by passing in a configurable Let me know if you'd like any more changes. |
| do { | ||
| let expected = ["convent", "conform", "constrict", "condone"] | ||
| let base = ["vent", "form", "strict", "done"] | ||
| checkSequence(expected, base.lazy.map { "con" + $0 }) |
There was a problem hiding this comment.
Should these also be wrapped?
There was a problem hiding this comment.
It would be best if they were, yes! Then you'd only need one test, because you would not care if the elements are strings or numbers.
|
@austinzheng Thanks again, I left more comments! |
|
|
||
| CollectionTests.addBidirectionalCollectionTests( | ||
| makeCollection: { (elements: [OpaqueValue<Int>]) in | ||
| MinimalCollection(elements: elements).lazy.filter { _ in return true } |
There was a problem hiding this comment.
This is also technically invalid. A MinimalCollection is not a BidirectionalCollection. It's almost certainly the same issue we discussed above with Sequence and addCollectionTests. My updated diff (coming soon) with gyb should fix this.
austinzheng
force-pushed
the
swift-3-indexing-model
branch
from
bcfe02a to
95fdcf7
Compare
| % for (specifier, kind) in variations: | ||
| CollectionTests.add${specifier}${kind}Tests( | ||
| make${kind}: { (elements: [OpaqueValue<Int>]) in | ||
| Minimal${specifier}${kind}(elements: elements).lazy.filter { _ in return true } |
There was a problem hiding this comment.
Thank you for the update!
Given the diagnosis in https://bugs.swift.org/browse/SR-1278, we can't really tell which overload is being called here (after all, Array is a bidirectional collection). To make sure, could you add an explicit return type to closures in these tests?
austinzheng
force-pushed
the
swift-3-indexing-model
branch
from
95fdcf7 to
6d78942
Compare
| // Test collections using value types as elements. | ||
| % for (traversal, kind) in variations: | ||
| CollectionTests.add${traversal}${kind}Tests( | ||
| make${kind}: { (elements: [OpaqueValue<Int>]) -> Minimal${traversal}${kind}<[OpaqueValue<Int>]> in |
There was a problem hiding this comment.
Isn't it something like ... -> LazyFilterCollection<Minimal${traversal}${kind}<[OpaqueValue<Int>]>>?
There was a problem hiding this comment.
Yep. I screwed this type signature up and it doesn't compile. Fixing...
|
@gribozavr Unfortunately I have to head out now. I'll work on this some more tonight and will ping you when it's ready; it'll definitely be done by tomorrow morning. There is an issue where the various checkXCollection functions all have different signatures. |
|
I'm finding it impossible to get (edited: some of) of the Filter tests to typecheck properly (note that this only occurs with the bidirectional collection test case): I don't get any compiler error messages. Instead, I get the following: Leaving out the return type annotations for the I'm going to be out for the next few days but I'll try to find a computer to finish this up from. In the meantime, any insight would be welcome. |
austinzheng
force-pushed
the
swift-3-indexing-model
branch
from
6d78942 to
a9e9a29
Compare
|
I've updated the PR with everything I have so far, but the Filter tests won't build properly yet and the PR shouldn't be merged until the issue is fixed. |
|
@austinzheng To try to work around the compiler bug, could you try breaking out closures into stand-alone functions? |
|
@gribozavr I tried defining the closures separately, as local variables, and wrapping everything in a |
|
I tried making the closures full functions and passing in function references, but it didn't do anything. I'll think about this some more. |
|
@gribozavr I was thinking: since this problem only occurs with the BidirectionalCollection version of the test, maybe I can dummy out the BidirectionalCollection variant of the test using GYB and insert a FIXME for when the issue is worked out. Let me know if you think that's an acceptable option. |
|
@austinzheng Absolutely it makes sense to unblock you and leave a FIXME. |
|
@austinzheng Actually, your test found a bug: Do you want to work on gyb'ing the two implementations? |
|
@austinzheng I'm on it. |
|
Thanks. Unfortunately I won't have access tp a computer until tomorrow night. Once this is in I'd like to work on improving the filter tests with pseudorandom data. That is probably best as a follow up PR. Let me know what you think. |
|
@austinzheng I have added diff --git a/validation-test/stdlib/Collection/LazyFilterCollection.swift.gyb b/validation-test/stdlib/Collection/LazyFilterCollection.swift.gyb
index 40ebaf5..77d455c 100644
--- a/validation-test/stdlib/Collection/LazyFilterCollection.swift.gyb
+++ b/validation-test/stdlib/Collection/LazyFilterCollection.swift.gyb
@@ -19,13 +19,13 @@ variations = [('', 'Sequence'), ('', 'Collection'), ('Bidirectional', 'Collectio
// Test collections using value types as elements.
% for (traversal, kind) in variations:
CollectionTests.add${traversal}${kind}Tests(
- make${kind}: { (elements: [OpaqueValue<Int>]) -> LazyFilter${kind}<Minimal${traversal}${kind}<OpaqueValue<Int>>> in
+ make${kind}: { (elements: [OpaqueValue<Int>]) -> LazyFilter${traversal}${kind}<Minimal${traversal}${kind}<OpaqueValue<Int>>> in
// FIXME: create a better sequence and filter
Minimal${traversal}${kind}(elements: elements).lazy.filter { _ in return true }
},
wrapValue: identity,
extractValue: identity,
- make${kind}OfEquatable: { (elements: [MinimalEquatableValue]) -> LazyFilter${kind}<Minimal${traversal}${kind}<MinimalEquatableValue>> in
+ make${kind}OfEquatable: { (elements: [MinimalEquatableValue]) -> LazyFilter${traversal}${kind}<Minimal${traversal}${kind}<MinimalEquatableValue>> in
// FIXME: create a better sequence and filter
Minimal${traversal}${kind}(elements: elements).lazy.filter { _ in return true }
},
@@ -37,7 +37,7 @@ CollectionTests.add${traversal}${kind}Tests(
// Test collections using reference types as elements.
% for (traversal, kind) in variations:
CollectionTests.add${traversal}${kind}Tests(
- make${kind}: { (elements: [LifetimeTracked]) -> LazyFilter${kind}<Minimal${traversal}${kind}<LifetimeTracked]>> in
+ make${kind}: { (elements: [LifetimeTracked]) -> LazyFilter${traversal}${kind}<Minimal${traversal}${kind}<LifetimeTracked>> in
// FIXME: create a better sequence and filter
Minimal${traversal}${kind}(elements: elements).lazy.filter { _ in return true }
},
@@ -47,7 +47,7 @@ CollectionTests.add${traversal}${kind}Tests(
extractValue: { (element: LifetimeTracked) in
OpaqueValue(element.value, identity: element.identity)
},
- make${kind}OfEquatable: { (elements: [LifetimeTracked]) -> LazyFilter${kind}<Minimal${traversal}${kind}<LifetimeTracked>> in
+ make${kind}OfEquatable: { (elements: [LifetimeTracked]) -> LazyFilter${traversal}${kind}<Minimal${traversal}${kind}<LifetimeTracked>> in
// FIXME: create a better sequence and filter
Minimal${traversal}${kind}(elements: elements).lazy.filter { _ in return true }
},This will make your test compile. |
|
@austinzheng We have merged the |
|
@gribozavr I'll open a new PR tonight when I get back home. Thanks for the update. |
What's in this pull request?
@gribozavr
This PR adds validation tests for
LazyFilterCollectionandLazyMapCollection. As a side effect, it also makesLifetimeTrackedformallyComparable.NOTE: The tests run but some of them fail. Not sure if it's because I am using the stdlib test APIs wrong or because there is an underlying bug with the new collection code, but given they only occur with one of the collection subtypes I'm inclined to believe it's the latter. I've reproduced the failing tests below:
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Note: Only members of the Apple organization can trigger swift-ci.