Fix queen-attack's #[should_panic] parameter

[gnuvince]

The .unwrap() method does not generate a panic string, so the test
always failed. The test can be fixed either by keeping the .unwrap()
call and removing the expected argument from the attribute, or by
keeping the attribute as-is, and using the .expect() method.

I've picked the former option to remain consistent with other tests (in
this problem and others) that use .unwrap().

[petertseng]

the test always failed

Well, there must be at least one implementation that is able to pass the tests, since otherwise the example solution would have failed the tests too. And we wouldn't have merged a PR without passing tests (otherwise there is a risk we could make unsolvable tests), so it is probably the case that the example solution passes the test.

It looks like after this was added in #119 we started preferring not explicitly checking error messages e.g. #147 (comment) and #169.

So on the same tack we should remove this expected from this should_panic. If you grep for all should_panic, the only other should_panic are in grains where we deliberately expect a panic, unlike queen-attack.

As an alternative, we may use is_err() instead, but note that this requires the student to use Result, whereas with the changed proposed in this PR the student may use both Result or Option (or even any other type that has an unwrap()...!)

Please, if anyone think we should use is_err instead, speak up. Otherwise I would suggest it be merged as-is (but maybe edit the commit message to reflect the reality instead of "the test always failed")

[IanWhitney]

If Err is constructed with a string, that string is returned as part of the panic. At least that was true when I wrote this code. I doubt that a recent Rust release has changed that behavior, as that would seem to be a breaking change.

I prefer the current approach because:

• The test structure mirrors the previous test, showing the two behaviors you can get when unwrapping a Result.
• The test illustrates how to write a should_panic test with a string expectation, which can be a handy technique
• The test require students to use Err with a message, also a handy technique

[gnuvince]

If the current approach is to be kept, then it would be preferable to offer a skeleton lib.rs file as some other problems do, or at least include a link to the relevant Rust book's chapter. In the 25+ previous problems I have completed, whenever a function or method returns a value of type Result<T,S> the error value is never used meaningfully and thus unit is the natural type to use.

(Also, as a general tip for writing Rust, don't use String as an error value: create an enum instead.)

[petertseng]

Ordered in increasing order of relevance to this particular PR:

don't use String as an error value

Doh, guilty. I may have to redo some submissions. https://doc.rust-lang.org/book/error-handling.html#defining-your-own-error-type agrees with what you say. And this leads very well into the next point.

---

error value is never used meaningfully and thus unit is the natural type to use.

It's agreed that it's natural if one writes minimal code to pass the tests. Obviously we would hope that a real library does not do this, but if we do not control what types the student might put into the error type, it is difficult to test.

On that note, we can see that if we use an enum for errors, we might be able to check that in the tests, such as in https://github.com/exercism/xrust/blob/master/exercises/circular-buffer/tests/circular-buffer.rs#L11. Other times, it is left to the student, for better or worse.

So as a side question, we may discuss: should more exercises' test suites in this track explicitly define enums for possible errors and then explicitly check that the correct enum variant is used?

Note that from what I know of the Swift track, they consistently do this. I don't have time to show y'all all the examples, but:

• https://github.com/exercism/xswift/blob/master/exercises/minesweeper/Tests/MinesweeperTests/MinesweeperTests.swift#L69
• https://github.com/exercism/xswift/blob/master/exercises/queen-attack/Tests/QueenAttackTests/QueenAttackTests.swift#L35
• https://github.com/exercism/xswift/blob/master/exercises/largest-series-product/Tests/LargestSeriesProductTests/LargestSeriesProductTests.swift#L52
• https://github.com/exercism/xswift/blob/master/exercises/bowling/Tests/BowlingTests/BowlingTests.swift#L103

---

The test illustrates how to write a should_panic test with a string expectation, which can be a handy technique

Should we double down on this by showing it twice, or is showing it once in https://github.com/exercism/xrust/blob/master/exercises/grains/tests/grains.rs#L46 sufficient?

---

with the changed proposed in this PR the student may use both Result or Option

After looking at the first few tests in this file (I should have done that instead of just looking at the changed)... Wow, I was completely wrong in this. There were already tests above that use is_ok and is_err.

That means that the test queen_panics_with_err_position has some portions redundant with previous tests:

• The previous test queen_is_created_with_a_ok_position already tells us that Queen::new takes a ChessPosition.
• The previous test chess_position_off_the_board_is_err already tells us that ChessPosition::new is a Result and it might return Err.
• This new test, since it calls unwrap, does three things:
  • Tests that unwrap() on an Err panics. This is not our job to test. That's the standard library's job.
  • Demonstrates to the student that the above is true. This can be a worthy cause especially if it is the first time we introduce Result (is it?), but I argue that we should have the minimal test that shows it, which means the Queen::new should not be a part of the test.
  • Tests the panic message. In the same way, if we want to test this, I recommend the minimal test that shows it: Just ChessPosition::new and unwrap, instead of muddying the waters by throwing in a Queen::new.

My recommendation is now:

1. If we still want to test the panic message, move the test above queen_is_created_with_a_ok_position, and remove Queen::new from the test, leaving only the ChessPosition::new(...).unwrap().
2. If we don't want to test the panic message, delete the test entirely.

[IanWhitney]

Just drop the test.

[IanWhitney]

Thanks for helping us improve the tests, @gnuvince!