Java: luhn: update tests and add version file

[FridaTveit]

The luhn tests should be updated to exactly match the canonical data. Also a version file should be added to match the canonical data version.

[sjwarner-bp]

I've just picked this one up, but our current test suite deliberately diverges from the canonical data (as per #972).

What would you recommend doing here @FridaTveit? 🙂

[FridaTveit]

From what I can tell we have one test (the one described in #972) which is not included in the canonical data but which is needed in our track. This test should have a comment on it (if it doesn't already) explaining why we keep it despite it not being in the canonical data and apart from that we should just make sure that all the other tests match the canonical data. Do you think that sounds like an okay plan @sjwarner-bp? 🙂

[sjwarner-bp]

Sure thing - that sounds good to me :slightly_smiling_face:

[FridaTveit]

Hi @sjwarner-bp, how's it going with this? Feel free to unassign it if you've got too much on your plate right now 🙂 Or keep working on it if you want, that's also fine 🙂

[sjwarner-bp]

Forgot I assigned myself this!

I am happy to free it up - it's been a hectic couple of weeks!

[FridaTveit]

That's fine, it's easy to forget! 🙂

[c-thornton]

So I am going to try to take care of this one tonight to clear it out. It looks like the canonical data has been updated in https://github.com/exercism/problem-specifications/commit/3930b0ac18b3155ff44b2ef9b32fe736e83d77b9 to include a test for the issue addressed in #972 in it now. I just want to double check that is the case and I can remove the previous test that diverges from the canonical data now and just follow it now. Can you let me know if I am understanding that correctly?

[FridaTveit]

@c-thornton That's great that you want to tackle this issue! 😃

However I think we still need the other test. We want to have a test case where there is a non-numeric character and using Character.getNumericValue to calculate the luhn sum will give an answer that is divisible by 10. In that case the algorithm should return false. Does that make sense?

I don't think the newly added test case satisfies those requirements. But actually, I'm not convinced the existing test case ("34&") does either. It's been a while since I did the exercise though so maybe I'm missing something. What do you think @sjwarner-bp @c-thornton @exercism/java? 🙂

[c-thornton]

@FridaTveit I checked the value returned from Character.getNumericValue('&') and it is -1 so that will result in a value that is not divisible by 10 (8 in this case I think). In order to get something that is divisible by 10 you would need to pass in something like ("85&") which will give 10 as the sum I believe. If you pass that in, I think that would be a good test as it should not pass if the program was set up correctly. I may be way off base on this though.

[c-thornton]

I also checked the test in the canonical data and ":9" will result in a total of 7 so I am not so sure if that is a valid test for issue #972 as well.

[FridaTveit]

@c-thornton yes, I think the test you suggest ("85&") will be a good test for the issue raised in #972 🙂 So in my opinion the changes needed are:

• Make any changes that have been made to the canonical data (including adding the new test)
• Change our test which deviates from the canonical data to use "85&" instead of "34&"

Do you agree @sjwarner-bp @c-thornton @exercism/java? 🙂

[c-thornton]

@FridaTveit That works for me and I think covers all of the bases here. If everyone else agrees I will get the changes made tonight and open a pull request.

[sjwarner-bp]

I'd say that makes sense and is a good way to attack this @FridaTveit :slightly_smiling_face:

Thanks for picking this one up @c-thornton :tada: