Triangle: test suite diverges from canonical data

[lbesaw]

The test diverges from canonical data here: https://github.com/exercism/problem-specifications/blob/master/exercises/triangle/canonical-data.json

[FridaTveit]

In my opinion (and I think this is an opinion other exercism maintainers share?) the tests in different tracks don't have to be exactly the same as the canonical data. As long as they're testing the same things and especially that they don't contradict each other, then I think it's fine for tracks to restructure the tests to more suit the language. For example the Java implementation of this exercise uses an enum to return the classification of the triangle, instead of having separate methods for checking whether a triangle is scalene, equilateral or isosceles which the canonical data might suggest. This would naturally result in slightly different tests. Do you agree that this is okay @exercism/java? Or should we change the tests to be more like the canonical data?

Was there anything in particular you think should be changed in the tests @lbesaw?

[stkent]

IMHO, having our test cases match 1:1 with canonical test cases (at least in spirit) is a good thing because it ensures thorough coverage. In fact, I'm such a fan of this that I generally avoid implementing any exercise without canonical tests.

Since Java doesn't auto-generate test suites, it is not unexpected that we sometimes fall out of sync with the canonical data. This is generally undesirable (though less so than in the past, now that language tracks "own" their READMEs also) and so we appreciate it being brought to our attention!

In this particular case, it looks like we will need to rethink the Java implementation fairly significantly in light of this new test:

equilateral triangles are also isosceles

Given that, we may want to introduce queryable methods (as hinted at by the canonical tests) and dump the classification enum.

[lbesaw]

My only real issue was that there is a test involving a degenerate triangle which is meant to fail in testing, however the instructions state that a degenerate triangle should pass. Just some consistency would be appreciated.

[FridaTveit]

@lbesaw , yes issue #799 covers that problem, which I absolutely agree is a problem. I don't particularly mind that our tests are slightly different to the canonical data, but I also don't mind reworking our solution if you think that's best @stkent. So having isIsosceles, isEquilateral and isScalene methods instead of the enum? I'm happy to open a PR for that :) I guess if we're then sticking 100% to the canonical data then we just don't test for the degenerate case at all?

[stkent]

@lbesaw you're right, the README for this exercise is currently "locked" at a version that doesn't match the test suite. This is a problem that will go away over time now that tracks "own" both pieces of data :)

@FridaTveit I like those method names, for sure; I don't see why we can't also perform tests for degenerate triangles, since it's called out as an option in the canonical data. It might be nice if the canonical data were updated to include sample degenerate tests with a comment regarding their optional nature, similarly to how the float tests are currently marked:

          "comments": [
            " Your track may choose to skip this test    ",
            " and deal only with integers if appropriate "
          ],

[FridaTveit]

Okay so the changes we're proposing are:

1. Changing the tests and example implementation to use isIsosceles, isEquilateral and isScalene methods instead of the enum.
2. Adding an isDegenerate method and adding tests for degenerate triangles.
3. Adding optional isDegenerate tests to the canonical data.

Does that sound okay @lbesaw, @stkent, @Smarticles101 and @jackattack24? I guess 1 and 3 should be done first (and can be done in parallel) but 2 shouldn't be done until 3 has been merged to make sure we're consistent with the canonical data? Do you want to work on any or all of those @jackattack24? You absolutely don't have to but I thought you should have the choice since you expressed interest in issue #799 :)

[jackattack24]

Sure, I can work on those! To change the canonical data I would change the canonical-data.json file for triangle in exercism/problem-specifications/exercises?

[FridaTveit]

I think that's right, and then you would have to open a PR to that repository :) Though @stkent would know more about that than me :)

[stkent]

Let's do them in order, 1, 2, 3, so we get a sense as to what the new tests should look like before we add them upstream. I can help you through that step when you get to it if you're interested, @jackattack24!

[Smarticles101]

Sounds good, I agree with @stkent's comment

[FridaTveit]

Thanks to @jackattack24, triangle now uses isIsosceles, isScalene and isEquilateral methods instead of an enum! :)

The next step I guess is to make our tests consistent with the canonical data. Do you want to do that as well @jackattack24? You absolute don't have to of course, you've done a great job already, but you can if you want to :)

[jackattack24]

@FridaTveit I'd be happy to work on it. So the next step would be adding isDegenerate and tests before we touch the canonical data, correct? (As per @stkent's last comment). I have ideas for some good tests, but I'm open to suggestions of course :)

And @stkent any help when we get to the canonical data would be greatly appreciated :)

[FridaTveit]

No, I think we should update the tests to be consistent with the current tests in the canonical data first. That way we'll solve the main issue with the README and tests being inconsistent. So we won't change the canonical data only update our tests to be consistent with it. Then we can add degenerate tests as an improvement. What do you think? :)

[jackattack24]

Ah okay. I already updated the test that threw an exception for a 2, 4, 2 triangle to test a 2, 5, 2 triangle in pull request #843. Otherwise, I think testing for equilateral triangles also being isosceles is the only other change necessary.

[FridaTveit]

I think our tests are still quite different. They don't use the same numbers as the canonical data and we have no tests for any of the methods returning false. As it is someone could pass those tests by just having all methods returning true (apart from when they need to throw an exception) :P Now that we're not using the enum anymore, we ideally want our tests to exactly match the canonical data. Does that make sense?

[jackattack24]

Yeah that makes sense :) Sorry for the misunderstanding

[FridaTveit]

No problem :)

[jackattack24]

Getting started on that now. Should I also be removing any existing tests that don't match the canonical data?

[stkent]

I think so; it's always easier to match canonical tests as closely as (practically) possible!

[jackattack24]

Just about ready with the changes, but I'm not sure whether or not to keep the following test:

public void trianglesWithNegativeSidesAreIllegal() throws TriangleException {
    expectedException.expect(TriangleException.class);
    new Triangle(3, 4, -5);
}

It seems like a pretty reasonable test to run, but testing negative sides isn't covered by the canonical data.

[stkent]

I vote for removal; I think we'd be better served as a track by having a few well-chosen exercises focus heavily on meaningful input validation.

[stkent]

Closed by #951 I believe!