Include Rex's collections-laws tests in PR validation

[SethTisue]

trafficstars

see discussion at https://github.com/scala/scala/pull/5891#discussion_r130339229

where e.g. @Ichoran wrote:

You can try running collections-laws for a start, which ought to catch these sorts of things, and says how good its coverage is. If it doesn't catch stuff like this, we should add a law. We also should include it in the automatic test suite before release, instead of having me intermittently running it manually. But I'm making it more friendly for other contributors (by making more of it Scala code instead of doing weird text replacement off of files with unique syntax), so maybe not just yet.

[SethTisue]

note that the current scala-collections-laws code is in @Ichoran’s fork, not (yet?) in the main repo under the scala org

the 2.12 community build includes a branch of that fork

[Ichoran]

It's there (in my repo only) because it's a work in progress. There are still collections that aren't covered, and methods that aren't covered, and the way to invoke it is pretty clunky. (Shell script calling two different sbt tasks.)

[SethTisue]

thanks, I'll try that branch.

the 2.12 community build includes a branch of that fork

as I look at this again for the first time in a long time, I note that the community build is just making sure that the repo compiles on latest Scala, it isn't running the full arsenal of tests. we need to add that to the Scala release steps.

[SethTisue]

note that the current scala-collections-laws code is in @Ichoran’s fork, not (yet?) in the main repo under the scala org

well that much is fixed for 2.12 via https://github.com/scala/scala-collections-laws/pull/18 and scala/community-builds@65a4be2f65049a166b41561820a54749b1e83cbe

[SethTisue]

scala/community-builds@a7a8a73b6dda003b04c081ade953442fae53aedd adds Rex's 2.13 branch to the 2.13 community build, where it is green. (but as with 2.12 as mentioned earlier, that just means it compiles, not that the tests pass)

[SethTisue]

got the scala-collections-laws 2.13.x branch running on my Mac. as the readme at https://github.com/Ichoran/scala-collections-laws/tree/only-new-coll-2.13 says, you have to publishLocal lihaoyi/sourcecode first, otherwise easy.

it takes about 10 minutes to run (faster than I'd expected/feared). there are currently a number of "Untested methods" warnings and a number of failures, more than a few, but not an overwhelming amount.

as an experiment, I did bash run.sh > out.txt against 2.13.0-pre-53c4be1 (the current M5 release candidate, unless we decide to rebuild), then I added VectorMap to Instantiator.scala and Generator.scala (copying the immutable.ListMap lines) and did bash run.sh > out2.txt, then inspected the output of diff -u out.txt out2.txt

I see

+Untested methods in ImmKV_VectorMap:
+  addString         aggregate         default           filterKeys        
+  flatten           fold              groupMapReduce    keySet            
+  keys              keysIterator      lazyZip           mapFactory        
+  product           reduceLeft        reduceLeftOption  reduceOption      
+  reduceRight       reduceRightOption remove            removeAll         
+  repr              scanRight         values            valuesIterator    
+  withDefault       withDefaultValue  withFilter        

which is similar (identical?) to what we see for the other maps, so no surprise there

there's a lot of further output that it's hard to fully evaluate just by eyeballing, but I did spot:

out2.txt:Fail((x.`-`(a._1)).`count`(_ == a) == 0
out2.txt:Fail((x.`-`(a._1)).`count`(_ == a) == 0
out2.txt:Fail((x.`-`(a._1)).`size` == x.size - (if (x.`contains`(a._1)) 1 else 0)
out2.txt:Fail((x.`-`(a._1)).`size` == x.size - (if (x.`contains`(a._1)) 1 else 0)
out2.txt:Fail((x.`-`(a._1)) partOf x
out2.txt:Fail((x.`-`(a._1)) partOf x
out2.txt:Fail(x.`size` > 0 implies (x.`init` sameAs x.`dropRight`(1))
out2.txt:Fail(x.`size` > 0 implies (x.`init` sameAs x.`dropRight`(1))
out2.txt:Fail(x.`size` > 0 implies (x.`init` partOf x)
out2.txt:Fail(x.`size` > 0 implies (x.`init` partOf x)
out2.txt:Fail(xsize < 1 || { x.`tail` sameAs x.`drop`(1) }
out2.txt:Fail(xsize < 1 || { x.`tail` sameAs x.`drop`(1) }

which shows that the laws already catch all three of the bugs in scala/bug#11100, which is the ticket that brought our attention back to this ticket here. yay scala-collections-laws!

finally, I ran it again with the Scala built from https://github.com/scala/scala/pull/7123 , with @mdedetrich's and my VectorMap fixes, and verified that those failures go away, as expected

anyway, I wanted to mess with this stuff and get a bit more familiar with it, mission somewhat accomplished.

[SethTisue]

anyway, clearly we need to get this stuff in better shape in time for 2.13.0-RC1.

the title of this ticket suggests making scala-collections-laws part of scala/scala PR validation. that seems ideal, but an easier, shorter-term goal would be for the Scala 2.13 community build to actually run these tests.

[Ichoran]

@SethTisue - I can help make this happen, but I'm not really the right person to get it into any particular place. If it's going into the main repo, it needs to run everywhere, but right now it relies on bash scripts which aren't sufficiently portable, and I never manage to get SBT to emulate make and/or bash. I have no expertise at all with the community build. So if I do it, I'm likely to spend a large amount of time stuck on some uninteresting problem that someone else who has familiarity could solve much faster than I could.

So could you figure out some other person to assign it to? I'll get collections-laws into good running shape with decent coverage. If nobody can do that, I could modify my run script to optionally (assuming a typical Unixy environment) automatically download and build the dependencies and make the needed changes so you can just type one thing and run the tests. That's probably better than having to follow the 10ish steps in the readme.

[SethTisue]

@Ichoran I think it will be sufficient for 2.13.0-RC1 to add it to the community build, PR validation is harder, maybe we'll tackle that later.

I can do the community build addition, but I'd appreciate it if you could do whatever you can to make it clean and well-documented before I do that.

[SethTisue]

note to self: when I do it, I should verify that adding VectorMap catches the bugs in https://github.com/scala/scala/pull/7357 (and say so on the PR)

wip: SethTisue/scala-collections-laws@eb54ed82ae534121f67c39e5c1701a8a9f7f497a

[Ichoran]

Okay, I'll get it passing (with flags to disable the things that don't work) and merge the working branch into master. It's already reasonably well-documented, but if there's anything in particular that should be better let me know (or file a bug report) and I'll try to improve it.

[Ichoran]

Master is now in the state that compiles and runs with no errors on my machine on 2.13.x. The repository doesn't seem to be heavily forked, so I just forced master to match my branch (saving the old master in old-2.12).

[SethTisue]

I should verify that adding VectorMap catches the bugs in scala/scala#7357 (and say so on the PR)

I'm assuming scala/scala-collections-laws#23 took care of this

[SethTisue]

and scala/community-builds@5afff0932d6f80f95836f5c4b58cdde9eeed9d54 takes care of the "add it to the 2.13 community build" part. which is enough for now

[Ichoran]

@SethTisue - It did take care of VectorMap!

But are you sure this works the way it's supposed to? You need to run laws/run prior to tests/test in order to create the laws, but you don't actually run them until you hit this line in the run.sh script:

sbt -J-Xmx6G -J-XX:MaxMetaspaceSize=4G -J-XX:-OmitStackTraceInFastThrow tests/test

I have no idea how the community build works, though, so maybe that information is somewhere else?

[SethTisue]

@Ichoran extra.test-tasks, which is separate from extra.commands, defaults to running test in all subprojects. if you want to be sure, you could look at the log at e.g. https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1637/consoleFull and make sure that the [scala-collections-laws] section is as you'd expect

[Ichoran]

Aha, okay, looks good then!