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

Ch 7 unit tests pass #146

Conversation

oldfartdeveloper
Copy link

@oldfartdeveloper oldfartdeveloper commented May 18, 2020

Issue #109

TLDR: I have all but three of the exercises covered by unit tests:

  1. Exercise Group 1 - Exercise 2: Convince yourself about lift3. This is proven by failed compilation; at some point I'll find a way to write a unit test that can respond to failed compilations.
  2. Exercise Group 3 - Exercise 1: The Traversable instance for a binary tree. I want to read more of the book so that I can write the tests for this as @milesfrain and @hdgarrood have suggested.
  3. Exercise Group 3 - Exercise 3: traverse implemented in sequence and vice-versa. I want to go on and come back to this.

Other Details

Initial Test

I needed to add

instance eqPhoneType :: Eq PhoneType where
  eq HomePhone HomePhone = true
  eq WorkPhone WorkPhone = true
  eq CellPhone CellPhone = true
  eq OtherPhone OtherPhone = true
  eq _ _ = false

to AddressBook.purs to get the unit tests to run. Hence, in addition I wrote a unit test to test this as the proof that unit tests are set up for this chapter.

Changes to AddressBook.purs and Validation.purs

Brought in @milesfrain simplifications to these two packages.

Brought in @milesfrain changes in branch master

So this PR will replace this branch's test/chapter7.md with the master branch one.

Still to be Done

  • I need to submit the master branch chapter 7 changes; will hopefully get that done today.
  • I'm still not sure how much to modify the exercises in the text to make it easier for the reader to understand that there are unit tests to verify his work. I know @milesfrain is working on this problem; should probably discuss w/ him.

milesfrain and others added 3 commits April 25, 2020 09:32
* formatted solutions

* Add solution for additional filtering tests

Co-authored-by: oldfartdeveloper <[email protected]>
@hdgarrood
Copy link

I needed to add instance eqPhoneType :: Eq PhoneType where ...

Did you know that you can write

derive instance eqPhoneType :: Eq PhoneType

and have the compiler write this code for you?

@oldfartdeveloper
Copy link
Author

@hdgarrood Dang, you're right; I forgot all about derive. I'll update, thanks!

@milesfrain
Copy link
Member

1. I'll find a way to write a unit test that can respond to failed compilations

I don't think that's possible, but don't worry about it. We can either just make a note about exercises without tests, remove the exercise, or replace with a new testable exercise.

2. The `Traversable` instance for a binary tree.  I want to read more of the book so that I can write the tests for this

I believe all the prerequisites for that are covered in Ch6, but feel free continue on with the book.

* I'm still not sure how much to modify the exercises in the text to make it easier for the reader to understand that there are unit tests

A guide on running tests will be part of Ch2 (PR #144), so I don't think we need to write much more about in in Ch7.

@milesfrain
Copy link
Member

I rebased the solutions branch on master to remove the distracting newtype wrapper changes from this PR. You'll want to rebase your solutions on the latest solutions branch too to re-sync everything.

@milesfrain
Copy link
Member

In order to not confuse readers when adding this to the AddressBook.purs file, I think it would be good to cover derive in Ch6 (#149). It may also be good to note that this is only added to facilitate testing.

derive instance eqPhoneType :: Eq PhoneType

@oldfartdeveloper
Copy link
Author

How about just a comment next to the statement containing the derive that this is needed only by the unit tests and the derive will be covered in chapter 10?

@milesfrain
Copy link
Member

Just a comment on derive is good for now

@oldfartdeveloper
Copy link
Author

@milesfrain After your rebase, there were some merge conflicts which I resolved. Travis passed.

@oldfartdeveloper
Copy link
Author

Per PR #147, modified this one's first unit test to match the master branch.

@@ -6,6 +6,25 @@ import Data.Maybe (Maybe(..))
import Data.Path (filename, root)
import Data.Tuple (fst)
import Effect (Effect)
import Test.Solutions
( allTrue
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like this is rebased correctly on the latest solutions (or master) branch, since there shouldn't be any changes here outside of ch7

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I've noticed (and wondered about) that as well. It's become tricky.

@milesfrain
Copy link
Member

I'll need to rebase solutions on master again. But to spare the hassle in coordinating that, feel free to just redirect this PR to the master branch, and I'll take it from there.
I didn't anticipate the added complications of involving multiple contributors in these rebases.

@oldfartdeveloper
Copy link
Author

When I switch the target to master, GitHub gives me a stern warning that this could be dangerous. It will probably break the build. I'll go ahead and switch the target to master and pray for your survival. ;)

@oldfartdeveloper oldfartdeveloper changed the base branch from solutions to master May 20, 2020 04:41
@milesfrain
Copy link
Member

@oldfartdeveloper
I made a bunch of exercise changes to Ch7 in #153 and also merged the solutions.
The remaining exercises should be a lot clearer now, so give them at shot at your convenience.

@milesfrain milesfrain closed this May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants