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");
}
\$\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\$ Jun 26 '15 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\$
    – Jakob
    Jun 26 '15 at 16:16
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
\$\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\$
    – Jakob
    Jun 26 '15 at 22:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.