I have a legacy system with a class with a lot of data. I am creating a web service that inserts a new claimant into that system for a contractor. I can not refactor the claimant object.
I need help on how to refactor my tests. I would never leave this as is but I wanted you to have the raw code to speculate about.
Current ideas:
- Create a special test for each field
- Create a test for each group of items
- Create helper methods for verifying a property to remove code duplication
The code:
[Test]
public void CreateClaimantInsertsAValidClaimantWithPropertiesFromRequest()
{
var claimantRepository = fixture.Freeze<Mock<IClaimantRepository>>();
var request = CreateValidClaimantRequest();
var sut = fixture.Create<ClaimantService>();
sut.CreateClaimant(request);
//Information
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Ssn == request.Ssn)), "Ssn");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Name == request.Name)), "Name");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.SupervisorName == request.SupervisorName)), "SupervisorName");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.SupervisorSsn == request.SupervisorSsn)), "SupervisorSsn");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.PostalCode == request.PostalCode)), "PostalCode");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.City == request.City)), "City");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Address == request.Address)), "Address");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Email == request.Email)), "Email");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Telephone == request.Telephone)), "Telephone");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Fax == request.Fax)), "Fax");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Category == request.Category)), "Category");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.BankAccountNumber == request.BankAccountNumber)), "BankAccountNumber");
//Contacts
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Contact1 == request.Contact1)), "Contact1");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.Contact2 == request.Contact2)), "Contact2");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.ServiceAdvisor == request.ServiceAdvisor)), "ServiceAdvisor");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.ServiceAgent == request.ServiceAgent)), "ServiceAgent");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.CollectionContact == request.CollectionContact)), "CollectionContact");
//Settings
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => Math.Abs(c.SubscriptionFeeSecondaryCollection - request.SubscriptionFeeSecondaryCollection) < 0.00001)), "SubscriptionFeeSecondaryCollection");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => Math.Abs(c.SubscriptionFeeInternational - request.SubscriptionFeeInternational) < 0.00001)), "SubscriptionFeeInternational");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => Math.Abs(c.SubscriptionFeeTotal - (request.SubscriptionFeeSecondaryCollection + request.SubscriptionFeeInternational)) < 0.00001)), "SubscriptionFeeInternational");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.ContractSigningDate == request.ContractSigningDate)), "ContractSigningDate");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.ContractResaleDate == request.ContractResaleDate)), "ContractResaleDate");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.UseDebtDeduction == request.UseDebtDeduction)), "UseDebtDeduction");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.HasSpecialNeeds == request.HasSpecialNeeds)), "HasSpecialNeeds");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.HasSpecialNeeds == request.HasSpecialNeeds)), "HasSpecialNeeds");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.IsSubjectToVat == request.IsSubjectToVat)), "IsSubjectToVat");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.HasDividedPaymentsInSecondaryCollection == request.HasDividedPaymentsInSecondaryCollection)), "HasDividedPaymentsInSecondaryCollection");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.UseDebtSurveillance == request.UseDebtSurveillance)), "UseDebtSurveillance");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DebtSurveillanceCommissionNumber == "050")), "UseDebtSurveillance");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.PaymentAllocationRule == PaymentAllocationRule.OldestFirst)), "UseDebtSurveillance");
//DebtEvaluation
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.IsDebtEvaluationActive == request.IsDebtEvaluationActive)), "IsDebtEvaluationActive");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DebtEvaluationMinAmount == request.DebtEvaluationMinAmount)), "DebtEvaluationMinAmount");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DebtEvaluationEmail == request.DebtEvaluationEmail)), "DebtEvaluationEmail");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DebtEvaluationType == DebtEvaluationType.ScoringAndCapital)), "DebtEvaluationType");
//DailySettlement
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.HasEmailForDailySettlement == request.HasEmailForDailySettlement)), "HasEmailForDailySettlement");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DailySettlementEmail == request.DailySettlementEmail)), "DailySettlementEmail");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.PaymentFileType == (Inkasso.Core.PaymentFileType)request.PaymentFileType)), "PaymentFileType");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.HasPaperSettlement == request.HasPaperSettlement)), "HasPaperSettlement");
//NewClaims
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.NewClaimsPostponementDays == 1)), "NewClaimsPostponementDays");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.NewClaimsEmail == request.NewClaimsEmail)), "NewClaimsEmail");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.NewClaimsTelephone == request.NewClaimsTelephone)), "NewClaimsTelephone");
//Frumkasso
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.IsFrumkassoEnabled == request.IsFrumkassoEnabled)), "IsFrumkassoEnabled");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.FrumkassoChainNumber == request.FrumkassoChainNumber)), "FrumkassoChainNumber");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DaysFromDeadlineToCollectionNotice == request.DaysFromDeadlineToCollectionNotice)), "DaysFromDeadlineToCollectionNotice");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DaysFromCollectionNoticeToSecondaryCollection == request.DaysFromCollectionNoticeToSecondaryCollection)), "DaysFromCollectionNoticeToSecondaryCollection");
//SpecialNeeds
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.MaxWeeksPostponement == 2)), "MaxWeeksPostponement");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.CanDebtorPayWithCreditCard == true)), "CanDebtorPayWithCreditCard");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.IsAgreementAllowed == true)), "IsAgreementAllowed");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.IsDebtorAllowedMultipleAgreements == true)), "IsDebtorAllowedMultipleAgreements");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.AgreementMaxMonths == 6)), "AgreementMaxMonths");
//Printing
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.DefaultNotificationPrintingMaterial == DefaultNotificationPrintingMaterial.Grayscale)), "DefaultNotificationPrintingMaterial");
claimantRepository.Verify(x => x.Insert(It.Is<Claimant>(c => c.IsDefaultNotificationStamped == true)), "IsDefaultNotificationStamped");
}
1 Answer 1
Ideally, one test case should be about one thing. One of the reasons for that is so that if something breaks, you will be able to pin point the problem, simply by looking at which of the cases failed. The more fine grained the tests, the quicker the debugging.
In a sense, this test is about one thing: verify the outcome of a valid request. But the problem is the class has too many fields, so this "one thing" is too big to be workable. One way or another, you need to break it down.
The obvious next level of detail to break this down might seem the fields: create one test per field. But that seems too detailed, not practical to implement, and might not even be all that useful.
So as you suspected yourself, I would group the fields. Most of the tests are actually very primitive, simply checking if the field in the request is the same as in the repository. There wouldn't be much benefit in testing these fields individually. If you name the test case to imply that the tests included are just trivial identity mapping tests, reviewers will understand that they don't need to read deeply into it, but mainly just look at the fields included in the tests. So even if such test case would be looking due to the number of fields, it could be still quite readable.
Some tests compare against default values. This could be another group.
A few tests compare against specific values. These could be turned into independent tests with descriptive names.
Some other coding style issues:
- magic numbers when comparing doubles: it would be better to move the common delta parameter to a constant. But the best would be to use a helper method to compare doubles
- conditions like
x == true
could be replaced with simplyx
itself
-
\$\begingroup\$ Thank you for your insight. One other thing. There is a lot of code duplication in this part: claimantRepository.Verify(x => x.Insert(It.Is<Claimant> would you leave it be or maybe create a helper method: VerifyInsertClaimant? \$\endgroup\$Jakob– Jakob2015年06月26日 22:35:13 +00:00Commented Jun 26, 2015 at 22:35
Claimant
class overrideEquals
/implementIEquatable
? Are the items you're testing fields, properties, or a mix? Are all public fields and/or properties (whichever it is you're testing) tested, or are some of them not relevant? \$\endgroup\$