emilybache / SupermarketReceipt-Refactoring-Kata

This is a refactoring kata for improving your coding skills
https://youtu.be/EWB-VhEUoHE
MIT License
102 stars 264 forks source link

Test coverage using Combination Approvals (csharp-ms) #36

Closed afhswe closed 1 year ago

afhswe commented 2 years ago

Hi Emily,

I forked your kata and created a branch providing 100% coverage with one simple approval test that is utilizing combination approvals. As one cannot create a pull request for adding a new branch I wanted to ask you if you want to include this branch into your kata. It's implemented for csharp-ms only for now, but we could add the tests for more languages (where available), such as Java.

The branch can be found here: https://github.com/afhswe/SupermarketReceipt-Refactoring-Kata/tree/with_combination_approval_tests

Please simply close the issue if you this does not make sense for your kata setup.

Best, Andi

emilybache commented 1 year ago

Hi, thanks for suggesting this. I like the idea of a branch showcasing a combination approvals approach here. I'm not sure it's quite ready to merge into my repo yet, I had a couple of questions.

I had a look at your code and I saw this kind of thing in the approved file:

[1,Each,TenPercentDiscount,10] => chocolate 1.00 chocolate 1.00 10% off(chocolate) -0.20

Total: 1.80

I wonder if you considered using a different printer or different printer configuration here? I'm concerned about the readability of a diff in case of failure. Did you think about just modifying the existing printer so it starts with a newline? Or if you thought about using a printer that only shows the short summary including the total price, and puts everything on one line?

I also wonder if you have any commentary or guidance about when this testing approach would be a good idea, or even a description of a learning hour where you would use this exercise?

afhswe commented 1 year ago

Hi Emily,

thank you for taking the time to review! Happy you like the idea :-)

Concerning your questions:

1. Approval file output format

You are right, this one was a little bit sloppy from my side. I adjusted the receipt printer to easier separate the items from each other with some formatting options (see latest commit in my forked repo branch. Please also note that I did not inject the options on purpose because I think there are reasonable default values for these two (adding a newline at the beginning and choosing the decoration text/character between receipt blocks)...

2. My take on when to use combination approvals

Throughout my career when working with different teams no matter what tech stack the question how to add tests to allow for safe refactoring legacy code by minimizing the effort for unit tests was always very present. Approval tests in general are great for that of course.

The reason why I think showcasing combination approvals particularly in your kata is that it demonstrates how quickly you can get to high coverage without having to think too much about the test cases and what the code does but are still already able to safely start refactoring. To be honest, I started with a very simple set of combinations without trying to understand the behaviour of the code too much. Writing several separate test cases and creating the combinations of parameters myself would have taking much more time and I would have had to trying understand the code much more and do whitebox testing.

I usually like to, if a test safety net and automated refactoring tools allow me to, refactor code to better understand it.

So bottom line I would use combination approvals (if possible) when I can to quickly start exploratory refactoring that allows me to understand the code much easier than trying to understand it from it's current structure and trying to discover the needed test cases on the go.

I don't have a learning hour in place for that yet, but I think combination approvals fits well with an exercise where there are a couple of input parameter combinations to cover and from the current structure (without refactoring to a better understandable shape) it would also be hard to identify the respective equivalence classes and boundaries. Going through the output of test combinations in the approval output file helps to identify representative test cases easier as well then.

codecop commented 1 year ago

I like showcasing different testing styles or tool variants. In the past I had issues with participants being confused if the repo contains different code or branches for given language. This is just for C#, maybe a separate repository only with C# code and different showcases would be a good idea. The showcases could be in different languages and different exercises each. Then maybe collect them as top level folders noting kata, language and testing strategy in folder name?

emilybache commented 1 year ago

I like what you did now with the printer, the output file is much easier to understand. I would like to include this somewhere in my repo. codecop suggests a C#-only repo. I think if I split this into langauge repos I will have too many repos (I already have too many repos actually). Instead of creating a new repo or a new branch, what do you think about including it as an addition to the with_tests branch? You could create a new class "SupermarketCombinationsTest" with this test case in it.

afhswe commented 1 year ago

Thanks for the input @codecop! Still, I do like the idea of keeping this in the same repository a little more :-)

@emilybache I added the tests to the suggested branch, for now just for csharp-ms and created a new pull request for that: https://github.com/emilybache/SupermarketReceipt-Refactoring-Kata/pull/41

afhswe commented 1 year ago

Thank you for the review! Closing the issue :-)