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

connect: adding examples for connect exercise #297

Merged
merged 12 commits into from
Jul 25, 2016

Conversation

jonatas
Copy link
Contributor

@jonatas jonatas commented Jul 19, 2016

It's related to the exercise on exercism/ruby#398.

Based on concerns presented on exercism/ruby#396.

@petertseng
Copy link
Member

petertseng commented Jul 20, 2016

Let's make sure that for everything mentioned in #212 this PR either addreses it or we explicitly decide it's not important.

In #212 a check for equal numbers of pieces was requested but I don't think it is useful to the problem, but maybe someone will disagree me on that.

I did a quick look and I think all tests here have equal side lengths so that solves one item noted in #212. Okay, there are some that are 4 and 5, but I also don't think is useful to the problem.

"description": "Rectangle white - white wins",
"board": [
". O . .",
"O X X X",
Copy link
Member

@petertseng petertseng Jul 20, 2016

Choose a reason for hiding this comment

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

It becomes hard to visualise the board when it i presented like this instead of

               . O . .
                O X X X
                 O O O .
                  X X O X
                   . O X .

The extra spaces make it clear that O has won, which isn't obvious (to me at least) without the extra spaces

@petertseng
Copy link
Member

petertseng commented Jul 20, 2016

All existing test suites for this exercise strip spaces before presenting the input to the function under test. That is, the spaces are to aid the human only. I believe it would be useful to note this in the test file. You can put it at "#": "note goes here" or similar (we have other json files that use the "#" key as a comment in this fashion)

Maybe this is not necessary - it's already noted in the README at https://github.com/exercism/x-common/blob/master/connect.md . I guess one reason it would still be helpful is just as a reminder to any who wish to implement it for a track.

@petertseng
Copy link
Member

What would you say to tests of a 1x1 board that has only an X, and a 1x1 board that only has an O? I notice most implementing tracks have that test

@petertseng
Copy link
Member

petertseng commented Jul 20, 2016

Also thinkign about PHP's test for an illegal diagonal https://github.com/exercism/xphp/blob/master/exercises/connect/connect_test.php#L148-L152

The point of that test is to make sure that of the eight possible directions (cartesian product of [-1, 0, 1], [-1, 0, 1] minus (0, 0)) only six are possible. A test for both of the illegal diagonals would probably be good. Edit: testing one implicitly tests the other since you can't determine what direction the user starts the search from

@jonatas
Copy link
Contributor Author

jonatas commented Jul 21, 2016

@petertseng thank you for the help and feedback.

About the 1x1 test: I implement all the tests initially inspired on go example. But I believe it test meaning is test if "the stone is on the edge". But I agree it's really strange. My description was poor, perhaps we need to improve or even remove it.

I think that a set of boards testing 1x1, 2x2, 3x3 and 4x4 could be a good help to understand too. What do you think?

I think the example "only edges does not make a winner" is implementing the same idea behind the illegal diagonal tests. Can you confirm?

" X . X . X",
" . X . X .",
" . X X . .",
" O O O O O"
],
"expected": "X"
},
{
"description": "Aspiral black wins",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is either "a spiral" or just "spiral"

@petertseng
Copy link
Member

About the 1x1 test: I implement all the tests initially inspired on go example. But I believe it test meaning is test if "the stone is on the edge".

I think many solutions are doing this by searching for a path, and a 1x1 grid tests for what happens when a start point is also an end point. So I see it as important to have.

I think that a set of boards testing 1x1, 2x2, 3x3 and 4x4 could be a good help to understand too.

It would be most helpful if they tested new cases that are not present in other sizes. If there are no such new situations in 2x2, 3x3, 4x4 boards it would not be necessary to add them. So now the question is: Are there any?

I think the example "only edges does not make a winner" is implementing the same idea behind the illegal diagonal tests. Can you confirm?

I don't think it does, because it doesn't reject the same kind of solution. (Also, can you explain what kind of solution "only edges does not make a winner" will reject?) I will say what I mean by solutions that an "illegal diagonal" example rejects.

When spaces are removed, a board like the below:

a b c
 d X e
  f g h

Gets turned into just:

abc
dXe
fgh

So, just going by string index, it looks like the 8 spaces surrounding X are a, b, c, d, e, f, g, h.
But since the board is laid out hexagonally, only b, c, d, e, f, g are actually adjacent to X.
a and h (corresponding to [-1, -1] and [+1, +1]) are not adjacent to X.

So, I hope for a test that rejects a solution that falsely believes a and h are adjacent to X; the solution would declare that there is some winner for that test, when the correct answer is that there is no winner.

@jonatas
Copy link
Contributor Author

jonatas commented Jul 22, 2016

Also, can you explain what kind of solution "only edges does not make a winner" will reject?

It was suggested by @Insti on the PR.

@Insti
Copy link
Contributor

Insti commented Jul 22, 2016

Also, can you explain what kind of solution "only edges does not make a winner" will reject?

My intention was for a test of a board that looked something like this:

OOOX
 X..X
  X..X
   XOOO

In order to detect solutions that just checked opposite edges without checking for through connections.

I don't think that is what the test ended up looking like, but it may no longer be necessary since there are now more tests for invalid connections.

I've not done the exercise myself yet, and figured that those who had would have a better idea of what the appropriate tests actually were. Once all the current changes are finalised I'll solve the exercise myself to double-check the tests.

@petertseng
Copy link
Member

detect solutions that just checked opposite edges without checking for through connections.

Ah. Good idea!

I like what I see in this PR currently.

Do we still want those 1x1 tests? I say yes, but someone might convince me otherwise.

This is a question that might not need to be answered in this PR, but sometime I forget whether black is X and white is O or the other way around... and also whether black plays top-to-bottom and white plays left-to-right or the other way around. So it was hard for me to determine whether a test description of "black wins" correctly goes with an expectation of "X". But I would say we don't need to find an answer to this before merging this PR since it seems to match what we have. I'll check these against an implementation in a few hours, or @Insti can beat me to it.

@@ -44,7 +44,7 @@
"expected": ""
},
{
"description": "Rectangle black - black wins",
"description": "X wins crossing from left to right",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these 'minor' changes are a big improvement. 👍

@Insti Insti changed the title adding examples for connect exercise connect: adding examples for connect exercise Jul 25, 2016
@Insti Insti merged commit 2c29767 into exercism:master Jul 25, 2016
@Insti
Copy link
Contributor

Insti commented Jul 25, 2016

Thanks @jonatas

petertseng pushed a commit to exercism/go that referenced this pull request Jul 31, 2016
* generating tests using connect.json from x-common

  exercism/problem-specifications#297

* returning X/O instead of black/white
  matches with JSON data better
  no need to remember which of X/O is black and which is white anymore!
  

* lines renamed to board 
  matches with JSON data better

* add testVersion = 2
  X/O and lines -> board mean existing solutions won't pass
emcoding pushed a commit that referenced this pull request Nov 19, 2018
We give the clients the nicity of not flooding them with errors and
failures when they fetch the lessons, but with this, we inform them how
they can control the skip behaviors, and how they can test individually
on demand.
fixes #297
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.

3 participants