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

Tests for CIS 194 homework assignments (1 to 6) #8

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

Tests for CIS 194 homework assignments (1 to 6) #8

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 15, 2017

I have added tests based in homeworks (1 to 6) of CIS 194 course. The reason for this pull request is that all tests that use doctest package exists. The doctest package definition is:

The doctest program checks examples in source code comments. It is modeled after doctest for Python.

It can cause confusion at the time of run tests, because usually tests source code are in other files and folders. I think that a better practice is to use a testing package for this tests. I choosed hspec package because I found this package in some Stack Builders repositories. The hspec package definition is:

Hspec is a testing framework for Haskell. It is inspired by the Ruby library RSpec.

I have used hspec package to migrate doctest tests to this testing framework.

Please check these tests and write any commentary if is necessary.

@@ -1,15 +1,44 @@
module Main
( main

Choose a reason for hiding this comment

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

I think we should keep this to be explicit about what the module exports

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem 👍

]
main = hspec $ do

describe "toDigits" $ do

Choose a reason for hiding this comment

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

Not sure if it's important, but what about the case for numbers starting with 0 like 01234

Copy link
Author

Choose a reason for hiding this comment

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

The signature of toDigits function is toDigits :: Integer -> [Integer]. Haskell don't use leading zeros, so toDigits 01234 == toDigits 1234.

it "returns an empty list for negative inputs" $
toDigits (-17) `shouldBe` []

describe "toDigitsRev" $

Choose a reason for hiding this comment

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

What does this function makes if a negative number is passed as argument? and like before, not sure if important but what if the number starts with 0

Copy link
Author

Choose a reason for hiding this comment

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

Checking homework 01, toDigitsRev function has the same tests as toDigits function, I'm going to add the corresponding tests.

it "converts positive integers to a list of digits reversed" $
toDigitsRev 1234 `shouldBe` [4, 3, 2, 1]

describe "doubleEveryOther" $ do

Choose a reason for hiding this comment

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

Not sure if important, but what happens with an empty list?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if it's necessary to add a test with an empty lists as input, because the exercise doesn't indicate this kind of input. I know that is necessary to test all cases (valid and invalid inputs) but this can produce confusion in a Haskell beginner when he test doubleEveryOther function and don't pass a test with an empty list.

@@ -1,15 +1,39 @@
module Main
( main
Copy link

@juanpaucar juanpaucar Feb 17, 2017

Choose a reason for hiding this comment

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

Again, I think it's a better idea to keep specific what the module exports

Copy link
Author

Choose a reason for hiding this comment

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

Solved 👍

@@ -1,15 +1,61 @@
module Main
( main
Copy link

@juanpaucar juanpaucar Feb 17, 2017

Choose a reason for hiding this comment

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

same as above, let's keep being specific about what a module exports

Copy link
Author

Choose a reason for hiding this comment

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

Solved 👍

it "evaluates expression without parenthesis" $
evalStr "2+3*4" `shouldBe` Just 14

it "evaluates wrong expression" $

Choose a reason for hiding this comment

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

can we make a case for a negative number too?

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to modify this tests to consider operations with negative numbers.

let testSat = parseExp lit add mul "(3 * 4) + (-5)" :: Maybe Mod7
testSat `shouldBe` Just (Mod7 0)

describe "compile" $

Choose a reason for hiding this comment

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

what happens with incorrect input?

Copy link
Author

Choose a reason for hiding this comment

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

parseExp function in Parser.hs file handles this case, producing Nothing for the inputs which are not well-formed expressions.

Choose a reason for hiding this comment

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

ok

@@ -1,15 +1,45 @@
module Main
( main

Choose a reason for hiding this comment

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

same as above about what the module exports

Copy link
Author

Choose a reason for hiding this comment

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

Solved 👍


describe "reify" $
it "makes an instance of Expr to ExprT type" $
reify (mul (add (lit 2) (lit 3)) (lit 4)) `shouldBe` ExprT.Mul (ExprT.Add (ExprT.Lit 2) (ExprT.Lit 3)) (ExprT.Lit 4)

Choose a reason for hiding this comment

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

please, split this line to make it more readable

Copy link
Member

@flandrade flandrade left a comment

Choose a reason for hiding this comment

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

@jmorocho Good job. I left some comments. Could you please take a look? Thanks for your contribution!

In addition, could you please run hlint?

describe "whatWentWrong" $
it "returns a list of the messages corresponding to any errors with a severity of 50 or greater, sorted by timestamp" $
pendingWith " whatWentWrong returns IO [String]"
-- testWhatWentWrong parse whatWentWrong "src/sample.log" `shouldBe` ["Way too many pickles","Bad pickle-flange interaction detected","Flange failed!"]
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho Could you please remove this comment? Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem 👍


it "gets a list of moves whit 3 discs" $
hanoi 3 "a" "b" "c" `shouldBe` [("a","b"),("a","c"),("b","c"),("a","b"),("c","a"),("c","b"),("a","b")]

Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho Could you please add a EOF space?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem 👍

it "returns a list of the messages corresponding to any errors sorted by timestamp" $
pendingWith "whatWentWrong returns IO [String]"
-- testWhatWentWrong parse whatWentWrong "src/sample.log"
-- `shouldBe` ["Way too many pickles","Bad pickle-flange interaction detected","Flange failed!"]
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho Could you please remove this comment?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem 👍

Copy link
Author

Choose a reason for hiding this comment

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

I have written the test of this function. Check please.

main = hspec $ do

describe "toDigits" $ do
it "converts positive integers to a list of digits" $
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho Some of these describe a context. Could you please verify and add context? http://hspec.github.io/writing-specs.html

describe "toDigits" $ do
    context "when digits are positive integers" $ do
      it "converts to a list of digits" $ do

Copy link
Author

Choose a reason for hiding this comment

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

You are right, the context is when digits are positive integers. I going to change this.

Copy link
Member

@flandrade flandrade left a comment

Choose a reason for hiding this comment

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

@jmorocho Good job so far. I left some comments regarding the use of context. This might be helpful: http://lmws.net/describe-vs-context-in-rspec

sumDigits [16, 7, 12, 5] `shouldBe` 22

describe "validate" $ do
it "returns True if an Integer is a valid credit card number" $
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho What about the following?

context "when the credit card number is valid
  it "returns True"

Copy link
Author

Choose a reason for hiding this comment

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

I changed the context in case of valid an invalid credit card number.

it "finds local maxima and gets 1 elements" $
localMaxima [2,3,4,1,5] `shouldBe` [4]

it "finds local maxima and not gets elements" $
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho "find local maxima and doesn't get elements"?

In addition, I think we're missing some context functions here. Why in some case do you get one element or two?

context "list.... "
  it "gets 2 elements"

Copy link
Author

Choose a reason for hiding this comment

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

Context added in case that exists at least one local maximum or none.

it "returns xor from a list with an odd number of True values" $
xor [False, True, False] `shouldBe` True

it "returns xor from a list with an even number of True values" $
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho context when list has an even number of elements?

Copy link
Author

Choose a reason for hiding this comment

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

Context added.

eval (ExprT.Mul (ExprT.Add (ExprT.Lit 2) (ExprT.Lit 3)) (ExprT.Lit (-4))) `shouldBe` (-20)

describe "evalStr" $ do
it "evaluates expression with parenthesis" $
Copy link
Member

@flandrade flandrade Feb 22, 2017

Choose a reason for hiding this comment

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

@jmorocho What about the following?

context "when an expression has parenthesis"  $ do
  it "evaluates the expression"

Copy link
Author

Choose a reason for hiding this comment

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

Added two nested contexts because when the expression is well-formed (with or without parentheses), the function return value of the expression and when the expression is not well-formed (with or without parentheses), return Nothing.

`shouldBe` ExprT.Mul (ExprT.Add (ExprT.Lit 2) (ExprT.Lit 3)) (ExprT.Lit (-4))

describe "parseExp" $
context "when provided with multiple data types" $ do
Copy link
Member

@flandrade flandrade Feb 22, 2017

Choose a reason for hiding this comment

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

@jmorocho I think it's better to have multiple contexts here. For example:

context "when provided with an Integer data type"  $ do
  it "evaluates the expression"

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Your are right, this test have multiple contexts (Integer, Bool, etc.).

compile "(3*4)" `shouldBe` ((Just [PushI 3,PushI 4,StackVM.Mul]) :: Maybe Program)

describe "withVars" $ do
it "evaluates all expression if variable exist in a operation of Expr class" $
Copy link
Member

@flandrade flandrade Feb 22, 2017

Choose a reason for hiding this comment

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

@jmorocho What about the following?

context "when the provided variable exists in an operation of Expr class" $ do
  it "evaluates the expression"

Copy link
Author

Choose a reason for hiding this comment

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

Good suggestion, thanks 👍

it "converts to a list of digits reversed" $
toDigitsRev 9876 `shouldBe` [6, 7, 8, 9]

it "returns an empty list for 0" $
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho I think we're missing some context functions. Perhaps:

context "when the digit is 0"
  it "returns an empty list"

Copy link
Author

Choose a reason for hiding this comment

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

Yes, is a good suggestion.

toDigitsRev (-1) `shouldBe` []

describe "doubleEveryOther" $ do
it "doubles every other number beginning from the right (even number of elements)" $
Copy link
Member

Choose a reason for hiding this comment

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

@jmorocho Perhaps:

context "when the list has an even number of elements"
  it "doubles every other number beginning from the right"  

Could you please verify for each case of doubleEveryOther?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@ghost ghost changed the title Tests for CIS 194 homeworks Tests for CIS 194 homeworks 1 to 6 Mar 29, 2017
@ghost ghost changed the title Tests for CIS 194 homeworks 1 to 6 Tests for CIS 194 homework 1 to 6 Mar 29, 2017
@ghost ghost changed the title Tests for CIS 194 homework 1 to 6 Tests for CIS 194 homework assignments (1 to 6) Mar 29, 2017
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