2
\$\begingroup\$

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:

  1. Create a special test for each field
  2. Create a test for each group of items
  3. 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");
}
janos
113k15 gold badges154 silver badges396 bronze badges
asked Jun 26, 2015 at 15:52
\$\endgroup\$
2
  • \$\begingroup\$ Some questions: Does the Claimant class override Equals/implement IEquatable? 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\$ Commented Jun 26, 2015 at 16:13
  • \$\begingroup\$ @BenAaronson No ´Clamant´ does not implement any kind of equality. I am only testing public properties and some are irrelevant. I want the error messages to say what property is incorrect. \$\endgroup\$ Commented Jun 26, 2015 at 16:16

1 Answer 1

1
\$\begingroup\$

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 simply x itself
answered Jun 26, 2015 at 17:39
\$\endgroup\$
1
  • \$\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\$ Commented Jun 26, 2015 at 22:35

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.