triangle: Add clarity to test descriptions

[Mehni]

Proposed fix to exercism/csharp#1253

The current [Test] names confused me when I was doing the exercise. It was also difficult to see which tests failed, because of long method names combined with a small window with the test results.

This PR changes the "Returns_true_if_the_triangle_is_equilateral_false_if_any_side_is_unequal" style in favour of a pattern that follows "equilateral_triangle_returns_false_if_any_side_is_unequal".

[Mehni]

Some things I considered but didn't do:

• Change the version. Reading the test-data-versioning, would this be a major change?
• Technically the "sides can be floats" test uses doubles for the C# track. Since this is likely language specific, I'm not certain what the correct naming would be.

[rpottsoh]

Change the version. Reading the test-data-versioning, would this be a major change?

The changes I believe constitute a patch version change. I think bin/check_versions should have taken exception to the version not being changed and caused Travis CI to fail. @petertseng did check_versions perform as expected in this instance?

Update: @petertseng it appears Travis CI did not run at all....

Technically the "sides can be floats" test uses doubles for the C# track. Since this is likely language specific, I'm not certain what the correct naming would be.

I think floats is OK. Sometimes the term real is used as well as a more language agnostic way to infer that a value not be an integer, or a string, etc. Perhaps a more concise way to make the statement could be sides can be of a floating point type.

[rpottsoh]

The version should be bumped to indicate a patch change I believe. I just realized that Travis CI was not executed on this PR. @kytrinyx is there something going on here that should be addressed?

[SaschaMann]

With #1473 in mind, perhaps the descriptions could be shortened a bit, e.g. all sides are equal instead of returns true if all sides are equal? The returned value is right below it anyway.

(Just a thought, not objecting to the proposed change here if others approve it)

[SaschaMann]

Closed and reopened to trigger a Travis build.

[Mehni]

@rpottsoh from what you posted, wouldn't it be a major version since it renames a "property" (test type)? Or am I misunderstanding the nomenclature, and does it fall under "key formatting"?

@SaschaMann while "the returned value is right below it" is true when looking at the Test.cs itself, the test methods themselves aren't in the same screen as the test results (which is when you're most interested in expected vs actual). Coming from the C#-track, the IDE has the test results in a different pane. If I do the recommend way of testing dotnet test, I get a CLI with just the results.

[SaschaMann]

@SaschaMann while "the returned value is right below it" is true when looking at the Test.cs itself, the test methods themselves aren't in the same screen as the test results (which is when you're most interested in expected vs actual). Coming from the C#-track, the IDE has the test results in a different pane. If I do the recommend way of testing dotnet test, I get a CLI with just the results.

That seems like an issue with dotnet tooling, not one that the common test spec needs to solve. I referred to that issue because shortening descriptions seemed to be a common wish in it and, imo, return true if (or true if) doesn't add a lot of info to the test data. The vast majority of test cases on this repo don't mention the expected result in the description.
But as I said, I leave it up to others to decide on this, just wanted to bring it up.

[Mehni]

I've re-read the documentation and changing the description of tests is indeed a patch increment. Thanks for your patience, @rpottsoh.

I've also removed the "expected result" to bring this more in line with the other exercises. Thanks for your feedback, @SaschaMann.

[rpottsoh]

I think saying first, second, third is a little cleaner than (1) (2) (3). I like your changes to the case descriptions. I want to take these three a little further.

[rpottsoh]

@rpottsoh from what you posted, wouldn't it be a major version since it renames a "property" (test type)? Or am I misunderstanding the nomenclature, and does it fall under "key formatting"?

@Mehni I think this still falls under patch, all of your changes only involve descriptions and comments.

Ha! I see you posted a comment that makes this one moot. 😄 Thanks for taking the time to streamline this exercise for all of us.

[Mehni]

ofc 😄

I think Exercism is one of the best ways to learn coding and wanted to contribute.

[rpottsoh]

If this PR is merged it should be squashed. This PR should remain open for a day or two to allow others a chance to comment and offer suggestions. If it is still open on March 24th I'll Squash and merge it.