Queen-Attack should have unit test to check if queens on same square

[srpeterson]

Hi - I'm going through the F# track. In the Queen-Attack module I think there should be a unit test to check and see if 'canAttack' returns false if queen1 and queen2 are on the same square:

[<Fact>] let `Queens cannot attack if on same square`` () = let whiteQueen = (4, 4) let blackQueen = (4, 4) canAttack blackQueen whiteQueen |> should equal false

'canAttack' cannot assume that it will be passed two different values for queen1 and queen2. In real life two pieces cannot occupy the same square at the same time, so 'canAttack' should take into account this edge case, in my humble opinion!

[iHiD]

cc @exercism/fsharp

[srpeterson]

Updated md in original comment so now shows full unit test function

[NobbZ]

Its currently not covered by the canonical data:

https://github.com/exercism/problem-specifications/blob/master/exercises/queen-attack/canonical-data.json

So it should be added there as well.

Though I'd not say, that the function should return false in this case, but instead signal an error for invalid input in the lannguages idioms.

On the other hand side, that would overcomplicate the exercise… I think we should just assume for this exercise, that such an invalid game state is guaranteed to not happen due to the surrounding game implementation.

[srpeterson]

I don't think it would over complicate the exercise. Having the queen1 = queen2 check (whether it signals an error or returns false) is easy to implement. I did it in my solution (even though there is no unit tests for it currently). In my opinion, it helps the developer learn to consider all edge cases.

See: https://exercism.io/tracks/fsharp/exercises/queen-attack/solutions/7e229407a0424297b5dc3d27cd6a1c9d

Just a suggestion, my F# Exercism friends!

[sshine]

I've moved the issue to exercism/problem-specifications even though it currently discusses a test idea as being implementable on the F# track in particular. This is because this repo gathers more attention to such matters. Feel free to continue the discussion in the context of the F# track. :-)

[petertseng]

To make this decision, observe that there are two possible interpretations for the job of the to-be-tested function:

• "Given these two positions, which the caller guarantees to describe the valid pair of positions of two queens on a chess board, can they attack each other?" - In this interpretation, the caller has violated the guarantee (two queens cannot occupy the same position); the behaviour is undefined. It would be incorrect to expect either true or false as the answer. This repo should choose one of the below actions:
  • Choose not to include this case.
  • Choose to include this case, expecting it to be an error.
• "Given these two positions, do they describe a valid chess board in which two queens can attack one another?" - In this interpretation, false is the correct answer.

To decide what action to take, it is first necessary to agree on which interpretation is to be used, or to agree to disagree.

[Calamari]

In the canonical data there are checks, if the positions are on the board and throwing errors if they aren't, if I am reading this correctly. So I would also think, that having two pieces standing on the same spot fail the same invariant, that it is a valid chess position. So I would opt for an error as well.

[yawpitch]

Isn't this rather covered by the discussion comment at the top of the canonical data?

Testing invalid positions will vary by language. The expected value of 'error' is there to indicate some sort of failure should occur, while a 0 means no failure. Some languages implement tests beyond this set, such as checking for two pieces being placed on the same position, representing the board graphically, or using standard chess notation. Those tests can be offered as extra credit.

Personally I don't think we need to add anything more at this level, but if we're going to add a test case then I'd also say it should error, rather than return False, as it's an illegal board position, no different than any of the other illegal positions that we currently require an error for.

[ErikSchierboom]

Personally I don't think we need to add anything more at this level, but if we're going to add a test case then I'd also say it should error, rather than return False, as it's an illegal board position, no different than any of the other illegal positions that we currently require an error for.

I agree with everything here. Personally, I also don't feel we need to be exhaustive and test all invalid cases, but I don't mind it too. It definitely should be an error if it is added.

[srpeterson]

I agree with Erick that not all invalid cases need to be tested, especially those far right edge cases that occur once in a blue moon. But I do think obvious invalid cases should be tested. IMHP two queens occupying the came position is also an illegal position that should have a unit test. In the current Queen Attack tests, there are tests for valid queen position, and +/- rows and columns. These are also illegal positions, yet test for a bool return value. Maybe those should also return an error?

[yawpitch]

@srpeterson in what language track is that? The canonical data currently has an error state expected for positions that underflow or overflow the board bounds, while boolean values are expected only for legal positions on the board.

[srpeterson]

This is the F# language track

[yawpitch]

This in the F# language track

Yeah, looks like the F# track diverged somewhat from the canonical data there ... it's treating all error states as False-y, where the canonical data treats them as error states.

[ErikSchierboom]

Yeah, we did that because we wanted to not have error checking in this exercise. Once the new Concept Exercises are in place, we can revert this.