'Consider this, you've chosen to show specific code which demonstrates mocking and yet there is not enough information or tests to verify that the system behaves the same. Sure we can see that an account has had a validation done along with being requested to activate. But I can't tell you if it is the same account as before the refactoring or if the same validation is being run against it.'
That is not true. The code was not covered by tests and now it is. The test for a state transition adequately covers the following business requirements:
Given a transition validator passes we expect the set state method to be called with the relevant state. We also expect the model to be saved.
Given a transition validator fails we do not expect to update the state or update the model.
'That is a risk I'm willing to take because like this change if the only reason for change is meeting principles and code coverage metrics I'd reject the change and request that the refactoring take place when a behavior change is needed.'
Given you have no domain knowledge it is not a risk for you to take. I also find it surprising that considering the unit tests cover the relevant paths and as mentioned cover business requirements that you think they serve only for coverage.
'This makes code more complex and increases the risk behavior changed.'
The code is not more complex. It serves the exact same function as before in a similar number of lines and using no more abstraction.
I think you have some fundamental misunderstandings about the code/testing.
'Had you just written a unit test against the original private method I would not have had concerns with changes to functionality. Yes it would have broken principles and we still wouldn't be testing if the account got its activation called. But we could have covered the account validation edge cases and failures, an area I think is at greater risk of being wrong or breaking from changes to code elsewhere.'
This is where you demonstrate the lack of knowledge mentioned above. As I already stated the account validation edge cases and failures would be handled by the unit tests for the actual class that handles the validation. This class only handles the transitioning. Writing tests for the validation would be done elsewhere. More troubling is your wish to test a private method.
I appreciate your concerns for my lack of domain knowledge and disregard for principals you hold. But I have my concerns that you aren't adequately testing the original behavior, and this response only reaffirms you don't understand my concerns because you're only focusing on one part of the original code.
As for the domain knowledge, you mentioned a refactoring gone awry. I'm claiming the refactor should not happen, why is domain knowledge relevant when I still don't let the bad code through?
The refactoring was necessary in order to make the code less tightly coupled and in order to allow it to be unit tested. Unit tests are impossible when I have concrete dependencies instantiated in my methods.
The unit tests for cover the business requirements and paths for this class. There would be a myriad of other tests covering the validation class but they are not relevant here.
I was not referring to the validation class. There is not enough code present to verify the correct validation is happening, sure you have tests in the validation class, but is that the class used?
I hope you have learned a lot in the last three years. You totally lack understanding of LSP, SRP and testing in general, yet have a pompous attitude to boot.
@Jessie - There is plenty of code present to verify the transition takes place and this is what we are testing.
I already provided the business requirements for the class. Which of these requirements does the code not test ?
There are two paths - both of these are adequately covered.
'sure you have tests in the validation class, but is that the class used?'
What do you mean by this - this demonstrates to me you do not understand how to write software. You can see from the code that the class is being used. It is injected and its responses determine whether the transition runs or not.
The actual validation in this case is being mocked because it is a dependency of this class.
Yes the validator is injected, but that is only the test code. Did the original calling code get updated to inject the correct class? I don't know because it isn't tested.
This is something a merge request for this refactor would answer (because the article is a tutorial) thus my comments that their is more happening then just tests being added.
Of course the calling code would have been updated to inject the correct class. This has its own tests as well.
I didn't add it because at some point the tutorial has a boundary and short of showing every call from input to output (including my DI container and framework code) I'm not going to cover every eventuality in a short post.
The tutorial was simply taking a single class and making it more testable.
I do understand the challenge with writing a consice tutorial, I'm trying to point out there is still risk introduced by this refactor.
I would actually love to see how you test that the correct verification object is injected in a unittest, I personally don't know how I'd do it.
We're a place where coders share, stay up-to-date and grow their careers.
We strive for transparency and don't collect excess data.