Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change: Change test case order to be TDD orientated. #1563

Closed
wants to merge 2 commits into from
Closed

change: Change test case order to be TDD orientated. #1563

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 31, 2019

I have rearranged the tests for this exercise to be more test driven development orientated as we would potentially want the easier test cases to come first so the user doing the exercise can write code to fail fast (eg. negative coins, 0 coins, etc) then build up to the difficult scenarios of using Lilliputian and Lower Elbonia Coins.

This is just a suggestion as I found the exercises as a great way of doing TDD - albeit the tests having already been wrote!

@yawpitch
Copy link
Contributor

yawpitch commented Aug 1, 2019

I'm not exactly against this change, but I'm also not at all convinced that the new order is any more TDD than the old order. Seems to me that it would be perfectly sensible to write the old order, based on the following user stories:

  1. We need something that can tell Bob what coin to give back if the change can be given with a single coin.
  2. We need something that can tell Bob what coins to give back if the change can only be given with multiple coins.
  3. Management called, we're opening a store in Lilliput. Go figure.
  4. Management called again, we're opening a store in Lower Elbonia... apparently Bob's from there.
  5. Someone asked Bob a stupid question ... we need to tell him not to give any change back for no cash tendered.
  6. Oh plotz! We never ran out of pennies before!
  7. How do you make change for negative money? Bob ... look, Bob ... is this how we ran out of pennies?

Basically the three tests you've brought forward are edge cases for unlikely, unexpected, and effectively invalid inputs ... it's natural for those to be added in later cycles of TDD, as they're discovered. The tests you've moved to the end, though, test the essential functionality of the normal case(s).

@ghost
Copy link
Author

ghost commented Aug 2, 2019

Thanks for the feedback. I do agree on the ordering in the view of the user stories you provided.

Just to justify my reasoning, I'm coming more from a difficulty perspective as these exercises are to help people learn/hone their skills in a programming language.

I was doing the exercise in Node and at first I though the exercise was easy by just keep using the highest denominator coin from the start. When I got to the Lilliputian and Lower Elbonia coins tests, the difficulty ramped up. After tacking those, the negative and invalid scenarios were trivial. That is why I moved the Lilliputian and Lower Elbonia coins tests to the end as where the real challenge lay.
Whilst looking at other submissions of solutions I noticed some fell through the entire code for the negative and zero scenarios. Which, while worked, simple conditionals at the beginning to fail fast on these scenarios would be much more efficient.

@yawpitch
Copy link
Contributor

yawpitch commented Aug 3, 2019

Thanks for the feedback. I do agree on the ordering in the view of the user stories you provided.

Just to justify my reasoning, I'm coming more from a difficulty perspective as these exercises are to help people learn/hone their skills in a programming language.

I was doing the exercise in Node and at first I though the exercise was easy by just keep using the highest denominator coin from the start. When I got to the Lilliputian and Lower Elbonia coins tests, the difficulty ramped up. After tacking those, the negative and invalid scenarios were trivial. That is why I moved the Lilliputian and Lower Elbonia coins tests to the end as where the real challenge lay.
Whilst looking at other submissions of solutions I noticed some fell through the entire code for the negative and zero scenarios. Which, while worked, simple conditionals at the beginning to fail fast on these scenarios would be much more efficient.

The reasoning is understandable, but additions to the canonical tests happen all the time when edge cases are discovered. For many of these the new test would by necessity be a trivial fix, but we've been appending the new test to the end rather than trying to maintain some sort of canonical order ... on any new test it's going to end up being a judgement call (or, really, a coin toss) as to the "correct" new order, so it's seemed simpler to merely append.

Also not assigning a fixed order is inline with the usual recommendation that tests in no way depend on each other, which means that they should, really, be able to be run in any order at any time and still provide meaningful assurance that the code still works.

Lastly, the test runners in many languages simply won't respect the order we present here in the canonical data anyway. For example in Python they'll end up running in alphabetical order based on the test case name, while in other languages they may or may not appear in presentation order ... it's really up to the language's test runner. Thus trying to maintain a specific order is going to be problematic at best.

Personally I'm inclined to say we don't even try, though I'm certainly open to counter-arguments. At any rate the canonical data is essentially locked at the moment per #1560, and since there's no bugfix here I'm thinking we should probably close this one for now.

Is that okay with you? I am sorry for the work you put in; I don't disagree with your reasoning, I just think we can't easily apply it to all the tracks via the canonical data.

@iHiD
Copy link
Member

iHiD commented Aug 5, 2019

Hello. Thank you for this PR - it's really appreciated.

This repo is currently on "lock-down" while we think through a few things. As such, I'm going to put this PR on hold for now, and we will come back to once we've worked through the issues with this repo that we need to address. You can read more here.

Sorry that you've hit us at this momentarily awkward juncture, and thank you again for the contribution.

@iHiD iHiD added the hold label Aug 5, 2019
@petertseng petertseng changed the title Change test case order to be TDD orientated. change: Change test case order to be TDD orientated. Sep 21, 2019
Base automatically changed from master to main January 27, 2021 15:31
@ErikSchierboom
Copy link
Member

@whatcoda Are you still interested in working on this PR?

@ghost
Copy link
Author

ghost commented Jan 12, 2022

No thank you, I'll close to clean up. Thank you everyone for the feedback!

@ghost ghost closed this Jan 12, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants