initial implementation of a more comprehensive test suite #99

andrewthad · 2018-03-20T21:17:56Z

Still a work in progress

treeowl · 2018-03-20T21:22:25Z

Data/Primitive/Array.hs

+  liftEq p a1 a2 = sizeofArray a1 == sizeofArray a2 && loop (sizeofArray a1 - 1)
+   where loop i | i < 0     = True
+                | otherwise = p (indexArray a1 i) (indexArray a2 i) && loop (i-1)
+


I just merged my eagerness patch, so you should use indexArray## here. Also, if we're adding Eq1 we should surely also add Ord1.

andrewthad · 2018-03-20T21:30:24Z

@treeowl I probably won’t be able to get back to this PR tomorrow. If you want, go ahead and take over it. I am pretty confident that we are in agreement about the improvements that still need to be made to it.

treeowl · 2018-03-20T21:34:21Z

I'm not sure when I'll be able to get to it either! Things are going to be pretty crazy for the next couple weeks, TBH.

…

On Tue, Mar 20, 2018 at 5:30 PM, Andrew Martin ***@***.***> wrote: @treeowl <https://github.com/treeowl> I probably won’t be able to get back to this PR tomorrow. If you want, go ahead and take over it. I am pretty confident that we are in agreement about the improvements that still need to be made to it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#99 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_ZgDPbpw3FXTbme3T9Z6kLjovPsfks5tgXTxgaJpZM4SyqtS> .

…ickcheck-test-suite

…SmallArray

… did a pretty cool trick with a fixed-point combinator, but it didn't work. I couldn't figure out how to make it work, so I just copied the implementation used for Array and adapted it to SmallArray.

…eArray. Also, improve the performance of ByteArray's fromListN function

andrewthad · 2018-03-21T16:15:29Z

It turns out that the IsList instance for ByteArray was wrong since foldrByteArray was wrong.

… older GHCs

treeowl

What are these fromList copies for?

…ve.SmallArray

andrewthad · 2018-03-21T19:23:35Z

@treeowl For some reason, github won't show me which code your comment refers to, so I'll try to explain more generally what's going on. On GHC 7.4.2, there is no IsList typeclass, so the Data.Primitive.SmallArray module doesn't end up exporting anything that let's us build a SmallArray from a list. Consequently, I have to copy their definitions into the test suite.

andrewthad · 2018-03-21T19:51:24Z

@RyanGlScott This appears to be passing in travis now. There's a bogus failure for GHC 7.8, but everything else is good. Please review when you get a chance. In the process of adding the test suite, it was discovered that <*> was incorrect for both Array and SmallArray and that the IsList instance for ByteArray was incorrect. These have been fixed so that the test suite would pass.

RyanGlScott

Thanks. It looks like this does quite a bit more than change the test suite—do you mind adding entries to the changelog describing what all have changed here?

RyanGlScott · 2018-03-21T19:54:04Z

primitive.cabal

  Build-Depends: base >= 4.5 && < 4.12
               , ghc-prim >= 0.2 && < 0.6
-               , transformers >= 0.2 && < 0.6
+               , transformers >= 0.4 && < 0.6


Why are you bumping the upper version bounds for transformers? (Note that that is what is causing the GHC 7.8 build failure in Travis.)

If we can keep the bounds down, maybe we can just deprecate the last couple versions altogether. The correctness and safety bugs are really piling up.

RyanGlScott · 2018-03-21T19:56:31Z

test/main.hs

+(?) = flip
+{-# INLINE (?) #-}
+
+smallArrayFromListN :: Int -> [a] -> SmallArray a


If this is literally copy-pasted from the main library, it might make sense to just export it from Data.Primitive.SmallArray proper.

treeowl · 2018-03-21T20:06:38Z

Data/Primitive/ByteArray.hs

+        go !ix (x : xs) = do
+          writeByteArray marr ix x
+          go (ix + 1) xs
+    go 0 ys


While you're replacing the IsList instance, I think you should make it safe by checking against the given size on each iteration. I'm adding that check to the polymorphic arrays.

I’ll do that. It’s a pretty inexpensive check, and this function should never be used in performance-critical situations, and segfaults are terrible.

treeowl · 2018-03-21T20:11:11Z

Oh, I see... You want newer transformers because the lifted classes changed... Hrmm. I think it might be better just not to run those tests with sufficiently elderly GHC versions. Or something. Gross. Can we avoid Eq1, etc., in the test suite by using FlexibleContexts?

treeowl · 2018-03-21T20:26:08Z

@RyanGlScott Who is going to decide what happens with the last two releases? Having bug-ridden instances in non-deprecated releases seems rather bad.

RyanGlScott · 2018-03-21T20:27:24Z

I don't have Hackage permissions for primitive, so there's nothing I can do about that.

treeowl · 2018-03-21T20:29:50Z

Ah, I guess @dolio will have to chime in at some point.

andrewthad · 2018-03-21T21:13:58Z

I'll need to think a little more about how to allow transformers-0.2 and transformers-0.3. I don't particularly want to change quickcheck-classes to use FlexibleContexts since that makes the type signatures less clear. I'll keep thinking about this and see if I can come up with a different solution.

andrewthad · 2018-03-21T23:42:22Z

So, what I think that I’m going to do is make quickcheck-classes able to build with even older versions of transformers. When built with these old versions of transformers (and a sufficiently old version of base) , it will not provide property tests for any of the higher kinded type classes. In primitive, I’ll just CPP our the functor, applicative, etc. tests when the dependencies are old. Does this sound like an acceptable approach? Practically speaking, I believe that the effect it will have will be that GHC 7.8.4 does not get tests for higher kinded type classes run (since that appears to be the only one where transformers < 0.4 is used).

treeowl · 2018-03-22T17:51:18Z

Have you updated the tests to check `foldl1` and `foldr1`, and at least make a decent attempt to check `Traversable` (e.g., using `Writer`, and also comparing `foldMapDefault` to `foldMap` and `fmapDefault` to `fmap`)?

…

On Thu, Mar 22, 2018, 1:21 PM Ryan Scott ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In changelog.md <#99 (comment)>: > @@ -1,3 +1,16 @@ +## Changes in version 0.6.4.0 + + * Add `Eq1`, `Ord1`, and `Show1` instances for `Array` and `SmallArray`. Don't forget to mention Read1 here! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#99 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_adU9SiGpRwJDr_c-Up8ji9kc7F5ks5tg92CgaJpZM4SyqtS> .

…plementation for Array and SmallArray.

andrewthad · 2018-03-23T12:30:23Z

Just pushed a commit (and a new version of quickcheck-classes) where all the Traversable laws are tested. Also, I'm testing foldl1 and foldr1 now. As you suspected, something was still wrong with foldl1 (it was doing a right-associated fold instead of left-associated), and I've fixed that.

andrewthad · 2018-03-23T14:26:32Z

Build is passing.

RyanGlScott · 2018-03-23T14:53:37Z

Data/Primitive/SmallArray.hs

  , Typeable
+#if MIN_VERSION_base(4,9,0) || MIN_VERSION_transformers(0,4,0)
+  , Eq1
+  , Ord1


Perhaps we should derive Read1 here as well?

Your thoroughness knows no bounds.

RyanGlScott · 2018-03-23T14:56:32Z

Excellent work, @andrewthad.

At this point, I really only have one additional comment, which is about dependencies. Due to the hacky nature of primitive's test suite, we cannot cache the builds of any library which depends on primitive. Thus, with the addition of quickcheck-instances, we have to build several additional libraries on every run of Travis, including semigroupoids and aeson, which are somewhat bulky.

Perhaps this is an acceptable cost—I'm not sure. But it is worth thinking about. What are your thoughts?

andrewthad · 2018-03-23T15:02:05Z

@RyanGlScott It looks like the deps on semigroupoids and aeson only come from quickcheck-classes. I can add a cabal flag for turning those off. Do you know how to get travis to pass a flag to a dependency? (I don't know how to use travis for cabal builds very well but I assume this is possible).

RyanGlScott · 2018-03-23T15:09:58Z

@andrewthad, that might be a worthwhile pursuit.

As far as how to change Travis, I think it would be a matter of passing --flags='quickcheck-classes -aeson -semigroupoids' to each invocation of cabal new-* in the script. The cleanest way to do this would be to declare:

- CABALFLAGS="--flags='quickcheck-classes -aeson -semigroupoids'"

Somewhere around here and pass ${CABALFLAGS} to the right places.

…ids to accelerate travis builds

RyanGlScott · 2018-03-23T15:47:12Z

.travis.yml


  # build & run tests, build benchmarks
-  - cabal new-build -w ${HC} ${TEST} ${BENCH} -j2 all
+  - cabal new-build -w ${HC} ${CABALFLAGS} ${TEST} ${BENCH} -j2 all


You'll need to pass ${CABALFLAGS} to more than just here. You'll also need it on lines 121, 124, 128, and 137, which call new-build, new-test, and new-haddock. (Otherwise, when those commands are run, it will first calculate a new set of dependencies, since you passed a different set of flags!)

Ah, thanks. I had forgotten that new-test even existed, so I didn't think to search for that. Also, it's a little strange to me that this to the earlier new-build commands in the file (the ones that don't build suite) would have any affect at all since primitive doesn't actually depend on quickcheck-classes.

To be clear, it's not necessarily guaranteed that not passing ${CABALFLAGS} would make a difference. But in my experience, things tend to work best when you use a consistent set of build flags everywhere—you don't want to give the recompilation checker any excuse for believing that some dependency needs to be rebuilt.

RyanGlScott · 2018-03-23T16:29:32Z

.travis.yml

  - HADDOCK=${HADDOCK-true}
  - INSTALLED=${INSTALLED-true}
  - GHCHEAD=${GHCHEAD-false}
+  - CABALFLAGS="--flags='quickcheck-classes -aeson -semigroupoids'"


I think I goofed—it turns out cabal-install doesn't like it when you surround this with double-quotes. I think it should work if you remove them.

andrewthad · 2018-03-23T17:05:05Z

I think I actually need to use constraint instead of flags. Trying this again.

RyanGlScott · 2018-03-23T17:20:36Z

No, flags should be correct. But I realized another issue—the quotes around the arguments to flags aren't being preserved. This should actually do the trick:

CABALFLAGS="--flags=\"quickcheck-classes -aeson -semigroupoids\""

I actually tested this one, so it better work :)

andrewthad · 2018-03-23T17:39:47Z

No dice.

RyanGlScott · 2018-03-23T17:46:17Z

Ugh. When I said "I actually tested this one", I meant I ran echo ${CABALFLAGS}, and it produced the output I expected. But evidently, this isn't what is actually passed to other commands... god I hate bash.

I'll experiment more with this locally.

RyanGlScott · 2018-03-23T17:59:32Z

On second thought, we might be making this way more complicated than need be. I neglected to notice that you actually amended cabal.project with the right flags in the first place! In light of that, I think this CABALFLAGS hack is wholly unnecessary. Can you try removing it and seeing if that does the right thing?

Sorry for the agonizing delay, @andrewthad—this stuff is always trickier than one would hope.

RyanGlScott · 2018-03-23T18:23:21Z

Oh, right. I forgot that haskell-ci generates its own cabal.project on the fly, which is why putting those flags in the local cabal.project won't show up. I know how to fix this, but this is something that can be done after merging.

In any case, this is building, and we should get this landed. Thanks for all the hard work, @andrewthad!

Follow-up to #99

andrewthad · 2018-03-23T18:39:47Z

Thanks!

treeowl · 2018-03-23T18:56:31Z

No, thank you. Testing is some of the least visible and most important maintenance work there is.

initial implementation of a more comprehensive test suite

576d062

andrewthad mentioned this pull request Mar 20, 2018

Add a proper test suite #89

Closed

treeowl reviewed Mar 20, 2018

View reviewed changes

andrewthad added 8 commits March 21, 2018 07:10

Merge branch 'master' of https://github.com/haskell/primitive into qu…

30f1756

…ickcheck-test-suite

fix laziness of Eq1 instance. Add Ord1 instance.

b18e93a

correct the definition of <*>

be53e00

Improve compatibility with older versions of GHC and transformers

190188b

add tests for SmallArray and ByteArray. add Eq1, Ord1, and Show1 for …

c7d3c77

…SmallArray

fix implementation of <*> for SmallArray. The previous implementation…

d1c3dfb

… did a pretty cool trick with a fixed-point combinator, but it didn't work. I couldn't figure out how to make it work, so I just copied the implementation used for Array and adapted it to SmallArray.

correct foldrByteArray, which fixes the IsList implementation for Byt…

2f13dea

…eArray. Also, improve the performance of ByteArray's fromListN function

improve compatibility with older transformers and base

19c67f7

andrewthad added 4 commits March 21, 2018 12:33

add tagged as dependency of test suite so Data.Proxy is available for…

6f39a7c

… older GHCs

use spaces instead of tab in test cabal file

d2ae384

guard isListLaws with CPP everywhere

a5bf17a

redefine fromList function for small array in test suite

bd5e8e1

treeowl reviewed Mar 21, 2018

View reviewed changes

remove the accidentally exported fromList functions from Data.Primiti…

1445345

…ve.SmallArray

RyanGlScott reviewed Mar 21, 2018

View reviewed changes

treeowl reviewed Mar 21, 2018

View reviewed changes

mention Read1 in changelog

2d00c3d

Start testing Traversable laws. Test foldl1 and foldr1. Fix foldl1 im…

229d0cb

…plementation for Array and SmallArray.

andrewthad mentioned this pull request Mar 23, 2018

Fold better; fix foldl1 bug #87

Merged

RyanGlScott reviewed Mar 23, 2018

View reviewed changes

derive Read1 when shimming SmallArray

46ba530

For test suite, build quickcheck-classes without aeson and semigroupo…

6f83f14

…ids to accelerate travis builds

RyanGlScott reviewed Mar 23, 2018

View reviewed changes

pass cabal flags to more places

567b4cb

RyanGlScott reviewed Mar 23, 2018

View reviewed changes

andrewthad added 2 commits March 23, 2018 12:50

remove double quotes from travis environment variable

552fc4c

try to make travis work again

ba0d7b8

fix cabal flags in travis file again

a57840d

remove CABAL_FLAGS from travis

cf9af6b

RyanGlScott merged commit 59e90f7 into haskell:master Mar 23, 2018

RyanGlScott added a commit that referenced this pull request Mar 23, 2018

Enable -aeson -semigroupoids on Travis

9df14ca

Follow-up to #99

treeowl mentioned this pull request Mar 26, 2018

The IsList instances for Array and SmallArray are unsafe #94

Closed

initial implementation of a more comprehensive test suite #99

initial implementation of a more comprehensive test suite #99

Uh oh!

Conversation

andrewthad commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewthad commented Mar 20, 2018

Uh oh!

treeowl commented Mar 20, 2018 via email

Uh oh!

andrewthad commented Mar 21, 2018

Uh oh!

treeowl left a comment

Choose a reason for hiding this comment

Uh oh!

andrewthad commented Mar 21, 2018

Uh oh!

andrewthad commented Mar 21, 2018

Uh oh!

RyanGlScott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treeowl commented Mar 21, 2018

Uh oh!

treeowl commented Mar 21, 2018

Uh oh!

RyanGlScott commented Mar 21, 2018

Uh oh!

treeowl commented Mar 21, 2018

Uh oh!

andrewthad commented Mar 21, 2018

Uh oh!

andrewthad commented Mar 21, 2018

Uh oh!

treeowl commented Mar 22, 2018 via email

Uh oh!

andrewthad commented Mar 23, 2018

Uh oh!

andrewthad commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanGlScott commented Mar 23, 2018

Uh oh!

andrewthad commented Mar 23, 2018

Uh oh!

RyanGlScott commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewthad commented Mar 23, 2018

Uh oh!

RyanGlScott commented Mar 23, 2018

Uh oh!

andrewthad commented Mar 23, 2018

Uh oh!

RyanGlScott commented Mar 23, 2018

Uh oh!

RyanGlScott commented Mar 23, 2018

Uh oh!