I appreciate this solution because it allows me to see some of that modern ruby pattern matching in action, but I do have to scrunch my nose a little bit at a solution that’s simply, “Make a big case statement and add another few cases to it.” This is generally how things get out of hand. OP addresses this later, and I do think the data-based approach is interesting, but I worry that it also approaches a design space that someone more junior might not be prepared to expand upon.
The solution that I’m used to seeing is this one from Sandi Metz, which not only covers how to create an open/closed solution but lays out the refactoring steps in more specific detail. Part of the benefit of this exercise is in demonstrating what the refactor process looks like in a situation where the code is just an absolute clusterfuck, and a big part of the lesson I try to impart to juniors that I do this exercise with is, “Trying to understand messy code can sometimes be a lesson in futility, but if you have tests already that you are confident with then you can ignore the old code and take incremental steps to refactor it away without explicitly understanding it.” (Obviously the followup question is, “What if I don’t have useful tests?” and that’s a different problem to solve.)
I’m also not a huge fan of this style of clustering tests, because the connotation of each test assertion isn’t provided by a description anywhere, as you normally have with rspec tests. It’s useful to see something like:
describe "standard item" do
it "reduces the quality by 1 when the sell_in date is still positive" do
...
end
it "reduces the quality by 1 when the sell_in date is reduced to zero" do
...
end
it "reduces the quality by 2 when the sell_in date becomes negative" do
...
end
it "cannot reduce the quality below 0" do
...
end
end
In the OP’s examples, they have those exact things tested with their assertions, but it takes a keen eye to make the comparisons to see what those assertions are actually testing. I don’t find that to be great for communicating the intent of the code.
I agree with what they have to say about not reaching for abstractions too early. I know the Sandi Metz example is definitely focused on an OOP approach, one which probably creates an abstraction too early and spreads things out into many disparate classes. Part of my philosophy when architecting systems is in trying to use a set of consistent, teachable patterns. To that end, I can teach a soft OOP approach as a lever developers can pull if they decide to start trying to encapsulate domain complexity, and I can’t do that as easily with other styles in ruby (yet). From what I’m seeing about a “stories-first” approach, it might become easier to identify when that lever should be pulled.
I do have to scrunch my nose a little bit at a solution that’s simply, “Make a big case statement and add another few cases to it.” This is generally how things get out of hand.
My point here is probably, “we all know the default way we’ll do this, wanna hear another opinion for a balance”? Because in my experience, in modern Ruby teams, things frequently get out of hand the opposite way: when people don’t even think of alternatives, immediately turning more-than-two-branches into a class hierarchy.
Too many times, I’ve seen chasing a small nasty bug or trying to add a seemingly trivial feature through 10+ classes invented years ago as the only “proper” way to organize code, when the answer to “where this wrong boolean value came from” is ten layers deep in a call stack from service to param handler to filter processor to transformer to decorator.
The usual answer to this is, “you just shouldn’t do bad OOP, you should do good OOP,” but I wanted to use this well-known kata to discuss the “keep it small while it is bearable” approach.
I’m also not a huge fan of this style of clustering tests, because the connotation of each test assertion isn’t provided by a description anywhere, as you normally have with rspec tests.
Oh, that’s one of my favorite beefs :) (And again, only shown here as an alternative to the approach “we all knew well.”)
My approach can be basically described as “the test code should be its own good description.” It is grown from many years of working with not-so-small production codebases, and my driving forces are (partially duplicating the text I’ve written many years ago and linked from this one):
with text-based description, nothing (other than administrative procedures) urges you to write good description, so many codebases have hundreds of tests with descriptions like “it works”, “it should return 1” (not providing the high-level description, just repeating the expectation), “it returns the proper color” and so on; with code-as-description, there is an incentive to write what exactly is happening here;
with text-based description, nothing urges you to write correct descriptions; tests get copy-pasted or edited all the time (with test body changed to contradict the description and making it useless); with code-as-description, if the code is correct, the description is correct;
with text-based descriptions, test code itself can easily grow large and clunky and there is much less incentive to make it clear (I already wrote a description, now those 20 lines do roughly what’s described!), and in the end of the day someone will need to understand and debug the code, not the description; with code-as-description there is much less chance of unclear clunky tests;
in general, this approach tends to produce tests that are very easy and fast to read, write, and extend, which contributes greatly to amount of cases covered (in more widespread approach, the thought of covering a couple more cases make you immediately anticipate laborous process of inventing the descriptions, setting things up, doing multi-line work you already did many times again and again); while thinking in subject-based one-expression tests frequently allow to extend the coverage with extreme ease.
What can I say… While understanding the drawbacks of the approach, I can testify it works, and being taught to teams with varying levels of experience improves the general readability and coverage of the test suite.
I’ve seen my fair share of problems in both directions: too abstracted and not abstracted enough, many times in the same codebase (or even the same file).
The problems I see most frequently with junior-level devs is “not abstracting enough” and “not knowing when to abstract” because those are the two things they simply don’t have experience with. We teach newbies if and else and then of course that becomes the hammer they hit all nails with. I can teach them other ways to handle conditionals, and I can teach them that they should hold off on using them until it makes their job easier. They can begin to start building simple abstractions from there, but probably will still miss opportunities to apply them.
The next level of problems I see with mid-level devs then is that they’ve been taught about “services” and “handlers” and “processors,” none of which actually do anything to encapsulate the domain effectively, but which do create many, many levels of indirection, as you’ve described. I can teach them to step back and avoid abstracting until a domain object under a describable pattern becomes clear, and I can describe to them a few patterns to apply to various types of domain objects based on the kinds of problems they’re likely to run into. They can begin to start seeing the shape of what real architecture looks like, though maybe not quite as well about when to architect more complex solutions.
Above that are the problems I see with senior-level devs, is that they often lack a holistic view of the system they’re working in. They’ll implement patterns based on what they know without seeking to understand what other patterns exist in the system and why. This comes from not necessarily being involved in the conversations with stakeholders around what kinds of outcomes we’re achieving, the tradeoffs we have to make to achieve those outcomes, and the amount of time we have to make it all happen. I can teach them what parts of the system need to be more robust and which ones it’s safe to be lazy on and why, but since the heuristics are contextual they might be lacking on the context to always get it totally right.
At my level, part of my job is in organizing the architecture in such a way that all three sets of developers are supported. To that end, there are times where I’ll prescribe an abstraction early, because I know that if I don’t it will get built out in a way that will grow out of hand quickly - as a bonus, I get an opportunity to expose someone to a pattern in a controlled environment, and then when it becomes more time sensitive that they know how to apply it they’ll be familiar with it already. Alternatively, if everybody’s already familiar with a pattern and we still have context being built by myself or by stakeholders, I’ll push to hold off on abstracting something even if it seems obvious that we should because there’s no additional value to doing it right this instant.
The point I’m hoping to make with all of this is that: yes, abstractions can very easily turn into a horrendous mess, but so can not abstracting and it’s entirely dependent on the people involved. I don’t believe we disagree on anything about the quality of code or even necessarily how to achieve it, but I do want to lay out that there’s not a technical solution for what’s ultimately a social problem: one of developer culture and how to teach and maintain it.
I believe this also applies to the writing of tests as well. I also want tests to be self-describing, but I also want someone to at least attempt to describe in words what it is they’re trying to demonstrate. There’s no perfect solution that guarantees that’s always going to work, but part of my job is in establishing the culture around me.
Tests with many assertions solve for one particular type of problem (seeing which assertions are contextually related to each other), but causes issues of another type (it’s harder to understand what each assertion connotes about the code under test). In the same vein of, “the descriptions can very easily be wrong,” I can see a similar tumor growing under the style you describe of, “we’ve got extra assertions that don’t validate anything new and they obfuscate the intended meaning of the test.” One might argue, “well that’s what PRs are for” and it’s like, yeah, that’s true with the descriptions as well. There isn’t a technical problem here, there’s a social one, and the correction mechanism is the same: someone with their head screwed on needs to be paying attention. I don’t think there’s any escaping that in software.
The stuff about naming tests reminds me a lot of the intuition Steve Klabnick wrote about recently that we should avoid giving things names where they aren’t needed.
I completely agree that I regularly see test names copy-and-pasted, or tests updated and the names left unchanged, or other issues that lead to names often being quite untrustworthy. And by “I regularly see this”, I of course mean that I do it myself if I’m being forgetful or not concentrating hard enough!
I’m trying to encourage my team to concentrate on making the code aspect of their tests clear, but it’s hard, partly because “write code for others to read” isn’t fully ingrained in their heads yet, but mainly I think because they go into autopilot when they start writing tests, and don’t think about what they’re trying to communicate with that test.
The one thing I didn’t entirely understand from your tests was the it_with logic. I assume that’s interacting with the before block, which is using the let-declared variable, but it looks very magical to my simple mind! Is this all part of RSpec or is this something extra going on? I’m not very used to Ruby and the Ruby ways!
The one thing I didn’t entirely understand from your tests was the it_with logic. I assume that’s interacting with the before block, which is using the let-declared variable, but it looks very magical to my simple mind! Is this all part of RSpec or is this something extra going on?
Yeah, I fast-forwarded on this part a bit, though I tried to explain it later (paragraphs starting with “This code uses saharspec, my RSpec extensions library (which tries to be ideologically compatible with RSpec, just taking its ideas further), namely its recent experimental branch where it_with method is implemented.”)
Basically, the idea is that in RSpec without any addons, you frequently need to write code like this (when testing branchy business logic):
subject { foo(argument) }
context "when argument is below 0" do
let(:argument) { -1 }
it { is_expected.to eq 123 }
# ↑ using implicit subject tests, same as...
it { expect(subject).to eq 123 }
end
context "when argument is 0" do
let(:argument) { 0 }
it { is_expected.to eq 456}
end
# ...and so on
…which, in the worst case, requires 4-5 lines to just state a simple correspondence of “this let value => that expected outcome” (and duplicating the “what is the context of the test” in the textual description). So, it_with is the simplification of such “static” contexts:
it_with(argument: 0) { is_expected.to eq 456 }
# is exactly the same as
context "when argument is 0" do
let(:argument) { 0 }
it { is_expected.to eq 456}
end
Are you implying that “Conjured Aged Brie” should increase quality twice as fast as “Aged Brie”?.. Nothing in the requirements suggests that. (But if this is the case, the code might be adjusted accordingly: then it turns out “Conjured” is not a separate class but a multiplier, so we actually have two variables now: quality change and change multiplier.)
I appreciate this solution because it allows me to see some of that modern ruby pattern matching in action, but I do have to scrunch my nose a little bit at a solution that’s simply, “Make a big case statement and add another few cases to it.” This is generally how things get out of hand. OP addresses this later, and I do think the data-based approach is interesting, but I worry that it also approaches a design space that someone more junior might not be prepared to expand upon.
The solution that I’m used to seeing is this one from Sandi Metz, which not only covers how to create an open/closed solution but lays out the refactoring steps in more specific detail. Part of the benefit of this exercise is in demonstrating what the refactor process looks like in a situation where the code is just an absolute clusterfuck, and a big part of the lesson I try to impart to juniors that I do this exercise with is, “Trying to understand messy code can sometimes be a lesson in futility, but if you have tests already that you are confident with then you can ignore the old code and take incremental steps to refactor it away without explicitly understanding it.” (Obviously the followup question is, “What if I don’t have useful tests?” and that’s a different problem to solve.)
I’m also not a huge fan of this style of clustering tests, because the connotation of each test assertion isn’t provided by a description anywhere, as you normally have with rspec tests. It’s useful to see something like:
In the OP’s examples, they have those exact things tested with their assertions, but it takes a keen eye to make the comparisons to see what those assertions are actually testing. I don’t find that to be great for communicating the intent of the code.
I agree with what they have to say about not reaching for abstractions too early. I know the Sandi Metz example is definitely focused on an OOP approach, one which probably creates an abstraction too early and spreads things out into many disparate classes. Part of my philosophy when architecting systems is in trying to use a set of consistent, teachable patterns. To that end, I can teach a soft OOP approach as a lever developers can pull if they decide to start trying to encapsulate domain complexity, and I can’t do that as easily with other styles in ruby (yet). From what I’m seeing about a “stories-first” approach, it might become easier to identify when that lever should be pulled.
My point here is probably, “we all know the default way we’ll do this, wanna hear another opinion for a balance”? Because in my experience, in modern Ruby teams, things frequently get out of hand the opposite way: when people don’t even think of alternatives, immediately turning more-than-two-branches into a class hierarchy.
Too many times, I’ve seen chasing a small nasty bug or trying to add a seemingly trivial feature through 10+ classes invented years ago as the only “proper” way to organize code, when the answer to “where this wrong boolean value came from” is ten layers deep in a call stack from service to param handler to filter processor to transformer to decorator.
The usual answer to this is, “you just shouldn’t do bad OOP, you should do good OOP,” but I wanted to use this well-known kata to discuss the “keep it small while it is bearable” approach.
Oh, that’s one of my favorite beefs :) (And again, only shown here as an alternative to the approach “we all knew well.”)
My approach can be basically described as “the test code should be its own good description.” It is grown from many years of working with not-so-small production codebases, and my driving forces are (partially duplicating the text I’ve written many years ago and linked from this one):
What can I say… While understanding the drawbacks of the approach, I can testify it works, and being taught to teams with varying levels of experience improves the general readability and coverage of the test suite.
Thanks for the thoughtful response.
I’ve seen my fair share of problems in both directions: too abstracted and not abstracted enough, many times in the same codebase (or even the same file).
The problems I see most frequently with junior-level devs is “not abstracting enough” and “not knowing when to abstract” because those are the two things they simply don’t have experience with. We teach newbies
ifandelseand then of course that becomes the hammer they hit all nails with. I can teach them other ways to handle conditionals, and I can teach them that they should hold off on using them until it makes their job easier. They can begin to start building simple abstractions from there, but probably will still miss opportunities to apply them.The next level of problems I see with mid-level devs then is that they’ve been taught about “services” and “handlers” and “processors,” none of which actually do anything to encapsulate the domain effectively, but which do create many, many levels of indirection, as you’ve described. I can teach them to step back and avoid abstracting until a domain object under a describable pattern becomes clear, and I can describe to them a few patterns to apply to various types of domain objects based on the kinds of problems they’re likely to run into. They can begin to start seeing the shape of what real architecture looks like, though maybe not quite as well about when to architect more complex solutions.
Above that are the problems I see with senior-level devs, is that they often lack a holistic view of the system they’re working in. They’ll implement patterns based on what they know without seeking to understand what other patterns exist in the system and why. This comes from not necessarily being involved in the conversations with stakeholders around what kinds of outcomes we’re achieving, the tradeoffs we have to make to achieve those outcomes, and the amount of time we have to make it all happen. I can teach them what parts of the system need to be more robust and which ones it’s safe to be lazy on and why, but since the heuristics are contextual they might be lacking on the context to always get it totally right.
At my level, part of my job is in organizing the architecture in such a way that all three sets of developers are supported. To that end, there are times where I’ll prescribe an abstraction early, because I know that if I don’t it will get built out in a way that will grow out of hand quickly - as a bonus, I get an opportunity to expose someone to a pattern in a controlled environment, and then when it becomes more time sensitive that they know how to apply it they’ll be familiar with it already. Alternatively, if everybody’s already familiar with a pattern and we still have context being built by myself or by stakeholders, I’ll push to hold off on abstracting something even if it seems obvious that we should because there’s no additional value to doing it right this instant.
The point I’m hoping to make with all of this is that: yes, abstractions can very easily turn into a horrendous mess, but so can not abstracting and it’s entirely dependent on the people involved. I don’t believe we disagree on anything about the quality of code or even necessarily how to achieve it, but I do want to lay out that there’s not a technical solution for what’s ultimately a social problem: one of developer culture and how to teach and maintain it.
I believe this also applies to the writing of tests as well. I also want tests to be self-describing, but I also want someone to at least attempt to describe in words what it is they’re trying to demonstrate. There’s no perfect solution that guarantees that’s always going to work, but part of my job is in establishing the culture around me.
Tests with many assertions solve for one particular type of problem (seeing which assertions are contextually related to each other), but causes issues of another type (it’s harder to understand what each assertion connotes about the code under test). In the same vein of, “the descriptions can very easily be wrong,” I can see a similar tumor growing under the style you describe of, “we’ve got extra assertions that don’t validate anything new and they obfuscate the intended meaning of the test.” One might argue, “well that’s what PRs are for” and it’s like, yeah, that’s true with the descriptions as well. There isn’t a technical problem here, there’s a social one, and the correction mechanism is the same: someone with their head screwed on needs to be paying attention. I don’t think there’s any escaping that in software.
The stuff about naming tests reminds me a lot of the intuition Steve Klabnick wrote about recently that we should avoid giving things names where they aren’t needed.
I completely agree that I regularly see test names copy-and-pasted, or tests updated and the names left unchanged, or other issues that lead to names often being quite untrustworthy. And by “I regularly see this”, I of course mean that I do it myself if I’m being forgetful or not concentrating hard enough!
I’m trying to encourage my team to concentrate on making the code aspect of their tests clear, but it’s hard, partly because “write code for others to read” isn’t fully ingrained in their heads yet, but mainly I think because they go into autopilot when they start writing tests, and don’t think about what they’re trying to communicate with that test.
The one thing I didn’t entirely understand from your tests was the
it_withlogic. I assume that’s interacting with the before block, which is using the let-declared variable, but it looks very magical to my simple mind! Is this all part of RSpec or is this something extra going on? I’m not very used to Ruby and the Ruby ways!Yeah, I fast-forwarded on this part a bit, though I tried to explain it later (paragraphs starting with “This code uses saharspec, my RSpec extensions library (which tries to be ideologically compatible with RSpec, just taking its ideas further), namely its recent experimental branch where it_with method is implemented.”)
Basically, the idea is that in RSpec without any addons, you frequently need to write code like this (when testing branchy business logic):
…which, in the worst case, requires 4-5 lines to just state a simple correspondence of “this
letvalue => that expected outcome” (and duplicating the “what is the context of the test” in the textual description). So,it_withis the simplification of such “static” contexts:I’m not convinced, what about Conjured Aged Brie?
Are you implying that “Conjured Aged Brie” should increase quality twice as fast as “Aged Brie”?.. Nothing in the requirements suggests that. (But if this is the case, the code might be adjusted accordingly: then it turns out “Conjured” is not a separate class but a multiplier, so we actually have two variables now: quality change and change multiplier.)