Skip to content
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

Added more unit tests around Shape #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ggranberry
Copy link

No description provided.

Copy link
Member

@cartazio cartazio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good,

specify "== on different shapes" $ (1:* 2:* 3 :* Nil) == (1:* 2:* 2 :* Nil) `shouldBe` False

specify "fmap on Nil" $ fmap (+1) Nil `shouldBe` Nil
specify "fmap on size n shape" $ fmap (+1) (1 :* 3 :* Nil) `shouldBe` (2:* 4 :* Nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i hadn't tested fmap? good point

specify "foldr on shape" $ S.foldr (+) 0 (1:* 2:* 3 :* Nil ) `shouldBe` P.foldr (+) 0 [1,2,3]
specify "foldl1 on shape" $ S.foldl1 (+) (1:* 2:* 3 :* Nil ) `shouldBe` P.foldl1 (+) [1,2,3]
specify "foldr1 on shape" $ S.foldr1 (+) (1:* 2:* 3 :* Nil ) `shouldBe` P.foldr1 (+) [1,2,3]

specify "== on Nil" $ Nil == Nil `shouldBe` True
specify "== on identical shapes" $ (1:* 2:* 3 :* Nil) == (1:* 2:* 3 :* Nil) `shouldBe` True
specify "== on different shapes" $ (1:* 2:* 3 :* Nil) == (1:* 2:* 2 :* Nil) `shouldBe` False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i nearly didn't see the 1 2 3 vs the 1 2 2

from a code coverage perspective, even a singleton list / shape should suffice i think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also i'd say "shapes with different element", :)

specify "map2 on shape" $ S.map2 (+) (1:* 2:* 3 :* Nil) (2:* 3:* 4 :* Nil) `shouldBe` (3:* 5:* 7 :* Nil)

specify "shapeToList on shape" $ shapeToList (1:* 2:* 3 :* Nil) `shouldBe` [1, 2, 3]
specify "shapeSize on shape" $ shapeSize (1:* 2:* 3 :* Nil) `shouldBe` length (shapeToList (1:* 2:* 3 :* Nil))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe i should define shapeSize in terms of the Foldable.length function? hrmmm

specify "backwards traverse on shape" $ backwards traverse (:[]) (1:* 2 :* Nil) `shouldBe` [(1:* 2 :* Nil)]

specify "unShapeVector on shape" $
do a <- return (uvFromList [1:* 2:* Nil,2:* 3:* Nil,3:* 4:* Nil :: Shape (S (S Z)) Int])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heere I think we're testing the Unboxed representation of a sized list (aka shape) right?

@@ -42,6 +88,20 @@ unitTestShape = describe "unit tests for Shape" $ do
do a <- return (uvFromList [3:* 4:* Nil,1:*2:*Nil :: Shape (S (S Z)) Int]) ;
UV.toList a `shouldBe` [3:* 4:* Nil,1:*2:*Nil]

specify "weaklyDominates equal to" $ (1:* 2:* 3 :* Nil ) `weaklyDominates` (1:* 2:* 3 :* Nil ) `shouldBe` True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, these are good to test too i suppose

@cartazio
Copy link
Member

cartazio commented Mar 3, 2018

some phrasing was confusing to me, possibly because i'm rusty.

that said: i do think you'd benefit from learning how to run the haskell code coverage tools, they let you see what branches of a function have been executed or not (which is of course in general isn't a good state space coverage metric, but is probably enough in this module, and i'm semi confident that at least you'd learn about whats actually adding coverage in a meaninful way vs more complex than needed :) )

@ggranberry
Copy link
Author

Thanks for taking a look! I had a similar feeling about some of the tests I was writing. I kept going back and forth about whether some of my cases were too trivial or even covering what the function was supposed to accomplish. I'll definitely look in the code coverage tools.

@cartazio
Copy link
Member

cartazio commented Mar 4, 2018 via email

@cartazio
Copy link
Member

cartazio commented Mar 4, 2018 via email

@cartazio cartazio force-pushed the master branch 3 times, most recently from 0f731a9 to 650ddea Compare March 15, 2020 22:04
@cartazio
Copy link
Member

i'll look at these again, past few years were nuts, pardon the slow folloow up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants