-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include Rex's collections-laws tests in PR validation #411
Comments
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.) |
thanks, I'll try that branch.
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. |
well that much is fixed for 2.12 via scala/scala-collection-laws#18 and scala/community-build@65a4be2 |
scala/community-build@a7a8a73 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) |
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 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 I see
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:
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 scala/scala#7123 , with @mdedetrich's and my anyway, I wanted to mess with this stuff and get a bit more familiar with it, mission somewhat accomplished. |
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. |
@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. |
@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. |
note to self: when I do it, I should verify that adding VectorMap catches the bugs in scala/scala#7357 (and say so on the PR) |
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. |
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). |
I'm assuming scala/scala-collection-laws#23 took care of this |
and scala/community-build@5afff09 takes care of the "add it to the 2.13 community build" part. which is enough for now |
@SethTisue - It did take care of 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:
I have no idea how the community build works, though, so maybe that information is somewhere else? |
@Ichoran |
Aha, okay, looks good then! |
see discussion at scala/scala#5891 (comment)
where e.g. @Ichoran wrote:
The text was updated successfully, but these errors were encountered: