Port junit tests to the strawman #200
Conversation
Fix ArrayBuffer constructor. Fix slice implementation.
Fix several issues with our implementation of LazyList. cf scala/scala#4284 and scala/scala#5259
Fix implementations of min and max of NumericRange.
Fix keySet hierarchy. Fix keysIteratorFrom method in BitSetOps.
Fix insertAll method of ArrayBuffer.
current implementation uses distinct thunks for the head and the tail, one can evaluate the tail of a LazyList without evaluating its head. In the case of a LazyList built from an Iterator, we still want to do the side-effect of calling `next()` when evaluating the tail, if the head hasn’t been evaluated.
Not really useful, though...
Add update method on mutable Sets. Add mkString method on Iterator.
Add some missing operators to mutable.BitSet.
Add collectFirst operation to IterableOps. Add withFilter, filterNot and collect to Iterator.
| } | ||
| else Iterator.empty.next() | ||
| } | ||
|
|
There was a problem hiding this comment.
I added a few missing methods to Iterator
| def firstKey: K = head._1 | ||
| def lastKey: K = last._1 | ||
|
|
||
| override def keySet: SortedSet[K] = new KeySortedSet |
There was a problem hiding this comment.
The keySet operation is now refined for sorted and immutable Maps to return a sorted or immutable Set.
| * @tparam A Element type (e.g. `Int`) | ||
| * @tparam CC Collection type constructor (e.g. `List`) | ||
| */ | ||
| abstract class WithFilter[+A, +CC[_]] { |
There was a problem hiding this comment.
I had to create this base WithFilter class as non inner class because it is extended by LazyList and it would mess with GC if it was an inner class.
| class LazyList[+A](expr: => LazyList.Evaluated[A]) | ||
| sealed abstract class LazyList[+A] | ||
| extends LinearSeq[A] | ||
| with LinearSeqOps[A, LazyList, LazyList[A]] { |
There was a problem hiding this comment.
I changed LazyList such that isEmpty and nonEmpty don’t evaluate the elements.
There was a problem hiding this comment.
And I fixed a bunch of methods to be GC friendly.
| override def toString: String = "Empty" | ||
| } | ||
|
|
||
| final class Cons[A](hd: => A, tl: => LazyList[A]) extends LazyList[A] { |
There was a problem hiding this comment.
The head and the tail are lazy and can be evaluated independently.
| * @define willNotTerminateInf | ||
| */ | ||
| final class NumericRange[T]( | ||
| sealed class NumericRange[T]( |
There was a problem hiding this comment.
I had to make this one non-final because of the private mapRange operation.
| // WrappedArray.make(res.asInstanceOf[Array[Unit]]) | ||
| // } | ||
| // } | ||
| // class RefWrappedArrayTest extends WrappedArrayTest[String](classOf[String]) with StringTestData |
There was a problem hiding this comment.
I commented these tests because we don’t have WrappedArray
| def its = vecs.map(_.toList.toIterator) | ||
| val cats = vecs.map(a => its.map(a ++ _)) | ||
| assert( cats == ans ) | ||
| } |
There was a problem hiding this comment.
I removed this one because concat now takes an Iterable, not an IterableOnce.
Remove TraversableLikeTest because we don’t have stringPrefix anymore in the strawman. Enable TraversableOnceTest.
| * @return an iterator which produces those values of this iterator which do not satisfy the predicate `p`. | ||
| * @note Reuse: $consumesAndProducesIterator | ||
| */ | ||
| def filterNot(p: A => Boolean): Iterator[A] = filter(!p(_)) |
There was a problem hiding this comment.
This is an inefficient way to do filtering. The fast way is to have a filterImpl that takes a boolean argument and defer to that with opposite sense for filter and filterNot (and make filter and filterNot final). Maybe it should be called filterWhen if it's part of the public API. Not sure this is the right time to make this change, but I can't think of a better time.
| protected[this] def fromSpecificIterable(coll: Iterable[K]): Set[K] = iterableFactory.fromIterable(coll) | ||
| protected[this] def fromSpecificIterable(coll: Iterable[K]): Set[K] = fromIterable(coll) | ||
| protected[this] def newSpecificBuilder(): Builder[K, Set[K]] = iterableFactory.newBuilder() | ||
| def diff(that: Set[K]): Set[K] = fromSpecificIterable(this).diff(that) |
There was a problem hiding this comment.
Can you explain how this works and what it's supposed to do? I'm puzzled.
There was a problem hiding this comment.
Basically we implement the diff operation by delegating to the IterableFactory defined above.
fromSpecificIterable(this) builds a Set[K] by using the IterableFactory defined a few lines above. This creates a copy of this keyset. Then we call its diff method.
There was a problem hiding this comment.
Oh, I see now that there's no default implementation of diff on Set; I thought there was, which is why I was so confused.
Still, this doesn't seem very efficient. Wouldn't something like fromSpecificIterable(view.filterNot(that)) make more sense, so you don't have to create the set twice?
| } | ||
|
|
||
| /** A generic trait that is reused by keyset implementations */ | ||
| protected trait GenKeySet extends Serializable { this: Set[K] => |
There was a problem hiding this comment.
Do you mean self instead of this? this is confusing.
There was a problem hiding this comment.
Why is this confusing? This is a self-type annotation.
There was a problem hiding this comment.
Yes, but if you have a nested class you then have to wonder whether the implicitly defined local this shadows the explicitly defined outer this. I guess if there isn't a nested class, it's fine; or we can expect people to know the rules regardless. Maybe this is a personal style I adopted (don't use this to name self-types; use _ if you don't need to name it, or some other name if you need to use it nested) that isn't widely followed.
| protected[this] def fromSpecificIterable(coll: Iterable[K]): SortedSet[K] = sortedFromIterable(coll) | ||
| protected[this] def newSpecificBuilder(): Builder[K, SortedSet[K]] = sortedIterableFactory.newBuilder() | ||
| protected[this] def sortedFromIterable[B: Ordering](it: Iterable[B]): SortedSet[B] = sortedFromIterable(it) | ||
| def diff(that: Set[K]): SortedSet[K] = fromSpecificIterable(this).diff(that) |
There was a problem hiding this comment.
Same thing--I don't understand how this works.
| } | ||
| } | ||
|
|
||
| /** Apply the given function `f` to each element of this linear sequence |
There was a problem hiding this comment.
This is too much functionality for me to review by eye and have much confidence I've caught a reasonable fraction of any errors. Hopefully I'll get collections-laws running soon.
| private[immutable] def mapRange[A](fm: T => A)(implicit unum: Integral[A]): NumericRange[A] = { | ||
| val self = this | ||
|
|
||
| // XXX This may be incomplete. |
There was a problem hiding this comment.
How do we find out if it is?
There was a problem hiding this comment.
I don’t know :(
I copied this from the existing code.
There was a problem hiding this comment.
I guess we need to add this case into collections-laws testing. Can you make a note somewhere that mapRange-generated ranges should be laws-tested, in case I forget?
|
Thanks @Ichoran I know its difficult to review… Maybe I should split this PR in several smaller ones? |
|
I think the big PR is okay; we just need decent testing at some point. By the end of next weekend I should have collections-laws running. |
|
FTR I’m also porting the scalacheck tests (which cover more operations than the junit tests). Do you think we should just switch to collections-laws? |
|
Collections-laws are fairly comprehensive but do not do a good job probing the corner cases that are appropriate for various different collections types. The unit tests do the latter well at least where breakages have occurred or been anticipated. Also, collections-laws were running too slowly to be run every compile. Don't worry too much about tests that are hard to get ported, though. |
|
I do understand the complementary value of unit tests and laws, I was more asking whether the scalacheck tests are still worth porting (since they may overlap with collections-laws). |
|
My recollection is that the scalacheck tests also covered some corner cases and also were not perfectly covered by collections-laws (that is, there wasn't really any major distinction between junit and not save for when they were written), but maybe put porting them on hold until we see how collections-laws do? |
|
OK, that works for me :) |
|
I just pushed 2 commits fixing your remarks, @Ichoran. |
|
@Ichoran Is this good to merge for you? |
|
Sorry--I'm behind as you've noticed. This was fine! |
2 participants
This PR is a first step towards porting the current collections tests to the strawman. I ported the JUnit tests found here (then I’ll work on the scalacheck tests).
To make the review easier I initially copy-pasted the tests without any changes in a tests-base branch, then I created a
testsbranch from there, where I incrementally enabled the tests and fixed things in the strawman if needed. The base branch of this PR is thetests-basebranch, so the diff you will see and comment on are the changes that I had to apply to the tests to make them work with the strawman.The tests that haven’t been ported are:
TrieMapTest: we first need to implementTrieMap(Add TrieMap collection type #195)scala.collection.convertandCollectionConversionsTest: we have no converters from or to Java collections yet (Explore how this library can support Java collections. #77)PagedSeqTest,QueueTest,LinkedHashMapTest,LinkedHashSetTest,MutableListTest,OpenHashMapTest,PriorityQueueTest,UnrolledBufferTest: we don’t have an equivalent of these collections (should we?)StringLikeTest: tests both sequence operations and non-sequence operations (e.g.toInt), which are not implemented in the strawmanSearchingTest: we have no equivalent in the strawman yet.I will add comments in the diff to explain the changes I’ve made.