Skip to content

connect: Rewrite test to use hspec with fail-fast. #298

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

Merged
merged 3 commits into from
Sep 18, 2016
Merged

connect: Rewrite test to use hspec with fail-fast. #298

merged 3 commits into from
Sep 18, 2016

Conversation

rbasso
Copy link
Contributor

@rbasso rbasso commented Sep 16, 2016

  • Rewrite tests to use hspec.
  • Update tests to match x-common.
  • Rename Color(Black,White) to Mark(Cross,Nought)
  • Patch the example and the stub solutions to use the new names.

Related to #211.

Now all the tests match x-common almost exactly. The only difference is that I kept the expected value as an ADT, not a String containing X or O.

Also, I had to change the ADT to have more meaningful names - how would anyone know that X was Black - so I borrowed them from Tic-tac-toe's Wikipedia page. The alternative would be to use X and O, but I'm not sure if it is a good idea to have single letter data constructors.

I can change the ADT if someone comes with better names.
Also, considering that we are breaking compatibility with old submissions, we could rename resultFor.

Updated (3 commits)
  • Rewrite tests to use hspec with fail-fast.
  • Update tests to match x-common.
  • Rename Color(Black,White) to Mark(Cross,Nought).
  • Rename responseFor to winner.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I do like the Black/White -> Cross/Nought change since I kept having to look at the README to remember which was which when I was doing this problem. I agree that the long names are slightly more desirable than X/O (single letter are harder to search for, maybe there are other reasons!)

I usually would have liked to put the Black/White -> Cross/Nought in its own commit, but in this case it would mean changing lines in the old tests that then promptly get replaced, so this way of doing it is not unwarranted.

What would you like for resultFor? winner?

@rbasso
Copy link
Contributor Author

rbasso commented Sep 17, 2016

I do like the Black/White -> Cross/Nought change since I kept having to look at the README to remember which was which when I was doing this problem.

The fact that the board uses X and O - both in README.md and canonical-data.json - but the text in README.md explicitly call them Black and White is confusing.

I'm not sure if I should have changed that here, because I removed all the references to colors in the exercise. Maybe it's a decision to be taken to x-common...

Should we open an issue about it there?

I usually would have liked to put the Black/White -> Cross/Nought in its own commit

You are right! I'll rewrite it, as a little punishment for my lack of discipline! 😄

What would you like for resultFor? winner?

I just had the feeling that resultFor was not good, but had no suggestion.
winner seems perfectly clear to me, and makes it obvious why it is a `Maybe. Nice choice!

I'm gonna change it in a separate commit.

@petertseng
Copy link
Member

I'm not sure if I should have changed that here, because I removed all the references to colors in the exercise. Maybe it's a decision to be taken to x-common...

Should we open an issue about it there?

Seems good. You probably have to do it as I will be occupied for a few hours.

@rbasso
Copy link
Contributor Author

rbasso commented Sep 18, 2016

Opened exercism/problem-specifications#380.

@rbasso
Copy link
Contributor Author

rbasso commented Sep 18, 2016

Thanks for the review!

Rewrote the PR in 3 separated commits, @petertseng.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

👍

@rbasso rbasso merged commit c7ea245 into exercism:master Sep 18, 2016
@rbasso rbasso deleted the hspec-connect branch September 18, 2016 04:19
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