function readOnly(count){ }
Sign Up ›
Starting November 20, the site will be set to read-only. On December 4, 2023,
forum discussions will move to the Trailblazer Community.
Learn More

Browse by Topic
ShowAll Questionssorted byDate Posted
+ Start a Discussion
SidViciousSidVicious 

Test Class for Trigger Handler Code Coverage not being captured

Hi everyone, my test class is finally coming around but in the end am on another jiffy. I was hoping if anyone have an insight as to why the code block below aren't being covered.

This is actually a continuation of my Previous Question (https://developer.salesforce.com/forums/?id=9062I000000IOUsQAO).

Apex class: 
public override void afterDelete() {

        for(InvoiceLine__c invLine : (List<InvoiceLine__c>) Trigger.old){

            if(invLine.InvoiceId__c != null){

                invLineIdSet.add(invLine.InvoiceId__c);
            }
        }

        List<AggregateResult> invoiceLineList = [SELECT COUNT(Id) counter, InvoiceId__c 
                                                 FROM InvoiceLine__c 
                                                 WHERE InvoiceId__c =: invLineIdSet
                                                 GROUP BY InvoiceId__c];

        List<Invoice__c> invoiceList = [SELECT Id, OpportunityId__c FROM Invoice__c
                                        WHERE Id IN: invLineIdSet];

        Map<Id, Boolean> invLineChecker = new Map<Id, Boolean>();

        Boolean noLineRecords;

        for(Invoice__c invL : invoiceList){

            noLineRecords = true;

            invLineChecker.put(invL.Id, noLineRecords);

            oppIdSet.add(invL.OpportunityId__c);
        }

        for(AggregateResult invoiceLine : invoiceLineList){

            Integer counter = (Integer) invoiceLine.get('counter');

            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
            
            if(counter > 0){

                noLineRecords = false;

                invLineChecker.put(parentInvoiceId, noLineRecords);
            }else{

                noLineRecords = true;

                invLineChecker.put(parentInvoiceId, noLineRecords);
            }
        }


        List<Opportunity__c> oppList = [SELECT Id, CurrencyISOCode 
                                        FROM Opportunity__c 
                                        WHERE Id 
                                        IN: oppIdSet];

        if(!oppList.isEmpty()){

            for(Opportunity__c opp : oppList){
                
                if(opp.CurrencyISOCode != orgCurr){

                    oppList1.add(opp.Id);
                }else{

                    oppList2.add(opp.Id);
                }
            }
        }


        invoiceCheckerList = [SELECT Id, OpportunityId__c FROM Invoice__c WHERE OpportunityId__c IN: oppList1];

        opptyMapp = new Map<Id, Integer>();

        invoiceRecWithoutLines = 0;

        for(Invoice__c invoiceCheck : invoiceCheckerList){

            if(invLineChecker.get(invoiceCheck.Id) == true){

                invoiceRecWithoutLines += 1;

                opptyMapp.put(invoiceCheck.OpportunityId__c, invoiceRecWithoutLines);
            }
        }


        for(Id key : opptyMapp.keySet()) {
            
            Integer countr = opptyMapp.get(key);

            if(countr > 0){

                oppListToUpdate.add(new Opportunity__c( Id=key,
                                                        InvoiceTotal__c=0,
                                                        InvoiceGrossProfit__c=0,
                                                        InvoiceDiscountTotal__c=0));
            }else{
                oppList3.add(key);
            }
        }

        if(!oppList3.isEmpty()){
            System.debug('SUCCESS!');
        }
}

I've highlighted the parts above which am not being covered. I'm having difficulty getting them covered in my Test class. The list  oppList3 aren't populated.

Here are the Test class I have so far: 
@isTest
    private static void deleteInvoiceLinesWithLineRecords(){

        initializeData();

        insert invoiceLineList;

        System.assertEquals(3, invoiceLineList.size());
		
		List<InvoiceLine__c> delInvList = [SELECT Id FROM InvoiceLine__c LIMIT 1];
		
		System.assertEquals(1, delInvList.size());

        Test.startTest();

        	try
			{
				delete delInvList;
			}Catch(Exception e)
            {
                String errormessage = e.getMessage();
                system.debug(errormessage);
            }

        Test.stopTest();

        delInvList = [SELECT Id FROM InvoiceLine__c];

        System.assertEquals(2, delInvList.size());
    }

    @isTest
    private static void initializeData(){
        
        opportunityRecord = new Opportunity__c();
        opportunityRecord.Account__c = '0012x000002kyl7AAA'; // required field
        opportunityRecord.CurrencyIsoCode = 'USD';
        opportunityRecord.Organisation__c = 'Default';
        opportunityRecord.InvoiceTotal__c = 0;
        insert opportunityRecord;
        invoiceRecord = new Invoice__c();
        invoiceRecord.OpportunityId__c = opportunityRecord.Id; // required field
        invoiceRecord.Account__c = '0012x000002kyl7AAA';
        invoiceRecord.TaxType__c = 'No Tax';
        invoiceRecord.TaxRate__c = 0;
        insert invoiceRecord;
        InvoiceLine__c invoiceLineRecord1 = new InvoiceLine__c();
        invoiceLineRecord1.InvoiceId__c = invoiceRecord.Id; // required field
        invoiceLineRecord1.DiscountType__c = 'Percent';
        invoiceLineRecord1.DiscountPercent__c = 5;
        invoiceLineRecord1.ProductType_Hidden__c = 'Discount';
        invoiceLineRecord1.Description__c = 'TEST';
        invoiceLineRecord1.Product__c = 'a042x00000BXXaZAAX';
        invoiceLineRecord1.Quantity__c = 1;
        invoiceLineRecord1.SalesPrice__c = 500;
        InvoiceLine__c invoiceLineRecord2 = new InvoiceLine__c();
        invoiceLineRecord2.InvoiceId__c = invoiceRecord.Id; // required field
        invoiceLineRecord2.DiscountType__c = 'Amount';
        invoiceLineRecord2.DiscountAmount__c = null;
        invoiceLineRecord2.Description__c = 'TEST';
        invoiceLineRecord2.Product__c = 'a042x00000BXXaZAAX';
        invoiceLineRecord2.Quantity__c = 1;
        invoiceLineRecord2.SalesPrice__c = 100;
        InvoiceLine__c invoiceLineRecord3 = new InvoiceLine__c();
        invoiceLineRecord3.InvoiceId__c = invoiceRecord.Id; // required field
        invoiceLineRecord3.DiscountPercent__c = null;
        invoiceLineRecord3.DiscountAmount__c = 0;
        invoiceLineRecord3.Description__c = 'TEST';
        invoiceLineRecord3.Product__c = 'a042x00000BXXaZAAX';   
        invoiceLineRecord3.Quantity__c = 1;
        invoiceLineRecord3.SalesPrice__c = 200;
        invoiceLineList = new List<InvoiceLine__c>();
        
        invoiceLineList.add(invoiceLineRecord1);
        invoiceLineList.add(invoiceLineRecord2);
        invoiceLineList.add(invoiceLineRecord3); 
    }
}

Again, any insights to this will be very much appreciated.

Please let me know if you need additional details.

Thank you!

- Sid
 
Best Answer chosen by SidVicious
Andrew GAndrew G
At first I was going to agree with Anudeep, and say that an invoice with no lines needs to be inserted.   But as I read the code for the n'th time, i realised that code will not be invoked for an invoice with no lines.   We are deleting Invoice lines and then wanting to do an update on the Oppty.
I would recommend a test method with a single invoice line and try delete that invoice line.

As I read the actual method, i find it quite convoluted and even after several readings, i'm still unclear on what its purpose is. However, we can simply some of it straight away (even without clarity of its purpose)

Part of this is redundant: 
Map<Id, Boolean> invLineChecker = new Map<Id, Boolean>();
        Boolean noLineRecords;
//this first loop set the Map to be <invoiceLineId, true>
        for(Invoice__c invL : invoiceList){
            noLineRecords = true;
            invLineChecker.put(invL.Id, noLineRecords);
            oppIdSet.add(invL.OpportunityId__c);
        }
//so as we enter this loop, every value is 'true'
        for(AggregateResult invoiceLine : invoiceLineList){
            Integer counter = (Integer) invoiceLine.get('counter');
            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
            if(counter > 0){
               noLineRecords = false;
                invLineChecker.put(parentInvoiceId, noLineRecords);
            }else{
//so why are we resetting the true value here?
                noLineRecords = true;
                invLineChecker.put(parentInvoiceId, noLineRecords);
            }
        }

we have this list variable, but it's not being used: 
oppList2.add(opp.Id);
additionally, we have this component 
for(Invoice__c invoiceCheck : invoiceCheckerList){
            if(invLineChecker.get(invoiceCheck.Id) == true){
                invoiceRecWithoutLines += 1;
                opptyMapp.put(invoiceCheck.OpportunityId__c, invoiceRecWithoutLines);
            }
        }
now for this component we went to the trouble of creating the invLineChecker Map which only purpose seems to be count lines.
Why not: 
for(AggregateResult invoiceLine : invoiceLineList){
            Integer counter = (Integer) invoiceLine.get('counter');
            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
    for(Invoice__c invoiceCheck : invoiceCheckerList){ 
        if(counter = 0){ 
            invoiceRecWithoutLines += 1; 
            opptyMapp.put(invoiceCheck.OpportunityId__c, invoiceRecWithoutLines); 
        }
    }
}
yes, its a horrible loop within a loop, but just as easily you could change the query to pass straight to the Map 
MAP<Id, Invoice> invoiceCheckerMap = [SELECT Id, OpportunityId__c FROM Invoice__c WHERE OpportunityId__c IN: oppList1];
and then do something like: 
for(AggregateResult invoiceLine : invoiceLineList){
            Integer counter = (Integer) invoiceLine.get('counter');
            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
    if(invoiceCheckerMap.containsKey(parentInvoiceId)){
        opptyMapp.put(invoiceCheckerMap.get(parentInvoiceId).OpportunityId__c, counter); 
    }
}


As for other feeback for your test class:

Use of hardcoded Ids is not best practice.  You are better off inserting all test data that is required for the test class.  
So something like: 
Account acc = new Account();
        	acc.Name = 'testAcc';
//set other required fields
        insert acc;

        opportunityRecord = new Opportunity__c();         
opportunityRecord.Account__c = acc.Id; // required field         
opportunityRecord.CurrencyIsoCode = 'USD';         
opportunityRecord.Organisation__c = 'Default';
opportunityRecord.InvoiceTotal__c = 0;         
insert opportunityRecord;

Since you are now looking for another test method, i would consider using @TestSetup
It allows for the creation of reusable test data with in the test class.  Where multiple Test methods are used, it is more efficient to user @TestSetup than creating a new suite of test data for each method.  And the test data is "reset" for each Test method.

Since you class is doing a delete, I would be testing with at least 2 x users to ensure the code behaves based on the user access.
And use of the System.runas(user); 
So here we have another test method that is required. An example of how to setup your users
  
 @testSetup static void setup() {
    String canDeleteId = [SELECT id FROM Profile WHERE Name='Can Delete records'].Id;
    String noDeleteId = [SELECT id FROM Profile WHERE Name='No Delete records'].Id;

 // Create 2 x user records 
    User userOne= createUser('user@One.net', 'userOne','test@none.net','userOne',canDeleteId ); 
    Insert userOne; 
    User userTwo= createUser('user@Two.net', 'userTwo','test@none.net','userTwo',noDeleteId ); 
    Insert userTwo;
  }
//other tests
    @isTest static void test_method() { 
        User user1 = [SELECT Id FROM User WHERE LastName = 'userOne'];
        System.runas(user1){
            //actual code to test
        }
    }

//create a method to quickly create the user if you don't have a utility class
	public static User createUser(String uName, String lName, String eAdd, String alias, String profileId) {
		User user = new User();
		user.Username = uName;
		user.LastName = lName;
		user.Email = eAdd;
		user.Alias = alias;
		user.TimeZoneSidKey = 'America/New_York';
		user.LocaleSidKey = 'en_US';
		user.EmailEncodingKey = 'ISO-8859-1';
		user.ProfileId =profileId;
		user.LanguageLocaleKey = 'en_US';
		return user;
	}
}


hope the above proves helpful

regards
Andrew
 

All Answers

AnudeepAnudeep (Salesforce Developers) 
Hi Sid - Looks like it is not entering the else part. Can you take another test method and ensure that opptyMapp is not populated?
  
Integer countr = opptyMapp.get(key);

opptyMapp requires Invoice__c so I recommend inserting any record of invoice__c type in the extra method
  
opptyMapp.put(invoiceCheck.OpportunityId__c, invoiceRecWithoutLines);

Let me know if it helps

Anudeep
 
Andrew GAndrew G
At first I was going to agree with Anudeep, and say that an invoice with no lines needs to be inserted.   But as I read the code for the n'th time, i realised that code will not be invoked for an invoice with no lines.   We are deleting Invoice lines and then wanting to do an update on the Oppty.
I would recommend a test method with a single invoice line and try delete that invoice line.

As I read the actual method, i find it quite convoluted and even after several readings, i'm still unclear on what its purpose is. However, we can simply some of it straight away (even without clarity of its purpose)

Part of this is redundant: 
Map<Id, Boolean> invLineChecker = new Map<Id, Boolean>();
        Boolean noLineRecords;
//this first loop set the Map to be <invoiceLineId, true>
        for(Invoice__c invL : invoiceList){
            noLineRecords = true;
            invLineChecker.put(invL.Id, noLineRecords);
            oppIdSet.add(invL.OpportunityId__c);
        }
//so as we enter this loop, every value is 'true'
        for(AggregateResult invoiceLine : invoiceLineList){
            Integer counter = (Integer) invoiceLine.get('counter');
            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
            if(counter > 0){
               noLineRecords = false;
                invLineChecker.put(parentInvoiceId, noLineRecords);
            }else{
//so why are we resetting the true value here?
                noLineRecords = true;
                invLineChecker.put(parentInvoiceId, noLineRecords);
            }
        }

we have this list variable, but it's not being used: 
oppList2.add(opp.Id);
additionally, we have this component 
for(Invoice__c invoiceCheck : invoiceCheckerList){
            if(invLineChecker.get(invoiceCheck.Id) == true){
                invoiceRecWithoutLines += 1;
                opptyMapp.put(invoiceCheck.OpportunityId__c, invoiceRecWithoutLines);
            }
        }
now for this component we went to the trouble of creating the invLineChecker Map which only purpose seems to be count lines.
Why not: 
for(AggregateResult invoiceLine : invoiceLineList){
            Integer counter = (Integer) invoiceLine.get('counter');
            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
    for(Invoice__c invoiceCheck : invoiceCheckerList){ 
        if(counter = 0){ 
            invoiceRecWithoutLines += 1; 
            opptyMapp.put(invoiceCheck.OpportunityId__c, invoiceRecWithoutLines); 
        }
    }
}
yes, its a horrible loop within a loop, but just as easily you could change the query to pass straight to the Map 
MAP<Id, Invoice> invoiceCheckerMap = [SELECT Id, OpportunityId__c FROM Invoice__c WHERE OpportunityId__c IN: oppList1];
and then do something like: 
for(AggregateResult invoiceLine : invoiceLineList){
            Integer counter = (Integer) invoiceLine.get('counter');
            Id parentInvoiceId = (Id) invoiceLine.get('InvoiceId__c');
    if(invoiceCheckerMap.containsKey(parentInvoiceId)){
        opptyMapp.put(invoiceCheckerMap.get(parentInvoiceId).OpportunityId__c, counter); 
    }
}


As for other feeback for your test class:

Use of hardcoded Ids is not best practice.  You are better off inserting all test data that is required for the test class.  
So something like: 
Account acc = new Account();
        	acc.Name = 'testAcc';
//set other required fields
        insert acc;

        opportunityRecord = new Opportunity__c();         
opportunityRecord.Account__c = acc.Id; // required field         
opportunityRecord.CurrencyIsoCode = 'USD';         
opportunityRecord.Organisation__c = 'Default';
opportunityRecord.InvoiceTotal__c = 0;         
insert opportunityRecord;

Since you are now looking for another test method, i would consider using @TestSetup
It allows for the creation of reusable test data with in the test class.  Where multiple Test methods are used, it is more efficient to user @TestSetup than creating a new suite of test data for each method.  And the test data is "reset" for each Test method.

Since you class is doing a delete, I would be testing with at least 2 x users to ensure the code behaves based on the user access.
And use of the System.runas(user); 
So here we have another test method that is required. An example of how to setup your users
  
 @testSetup static void setup() {
    String canDeleteId = [SELECT id FROM Profile WHERE Name='Can Delete records'].Id;
    String noDeleteId = [SELECT id FROM Profile WHERE Name='No Delete records'].Id;

 // Create 2 x user records 
    User userOne= createUser('user@One.net', 'userOne','test@none.net','userOne',canDeleteId ); 
    Insert userOne; 
    User userTwo= createUser('user@Two.net', 'userTwo','test@none.net','userTwo',noDeleteId ); 
    Insert userTwo;
  }
//other tests
    @isTest static void test_method() { 
        User user1 = [SELECT Id FROM User WHERE LastName = 'userOne'];
        System.runas(user1){
            //actual code to test
        }
    }

//create a method to quickly create the user if you don't have a utility class
	public static User createUser(String uName, String lName, String eAdd, String alias, String profileId) {
		User user = new User();
		user.Username = uName;
		user.LastName = lName;
		user.Email = eAdd;
		user.Alias = alias;
		user.TimeZoneSidKey = 'America/New_York';
		user.LocaleSidKey = 'en_US';
		user.EmailEncodingKey = 'ISO-8859-1';
		user.ProfileId =profileId;
		user.LanguageLocaleKey = 'en_US';
		return user;
	}
}


hope the above proves helpful

regards
Andrew
 
This was selected as the best answer
SidViciousSidVicious
Thanks to the replies, guys.

Hi @Andrew G,

I'm interested with your answer. Can you help me understand by writing your answer for the Apex class revision as a whole/together because I'm confused as to where to insert them or which of codes should I remove.

Thanks again.
Andrew GAndrew G
With no clear understanding of the purpose of the code, it is hard to do a complete revision.  What I have pointed out was aspects of your code that did not seem to make sense.  There seems to be a lot of back and forth between lists and maps of various objects.

If I was to make an educated guess, the purpose of the code:

When an Invoice Line is deleted, if it is the last Invoice Line in the Invoice, on the Opportunity reset values for InvoiceTotal__c, InvoiceGrossProfit__c, & InvoiceDiscountTotal__c to zero
 
I am assuming that your data structure only allows one Opportunity per Invoice, and obviously one to many for Invoice to Invoice Lines.

If so, below is what i believe would be a more readable piece of code 
public override void afterDelete() {

//get all invoice Ids for deleted Lines
    for(InvoiceLine__c invLine : (List<InvoiceLine__c>) Trigger.old){
        if(invLine.InvoiceId__c != null){
            invLineIdSet.add(invLine.InvoiceId__c);
        }
    }

//aggregate list of invoice lines, by Invoice Id & count
    List<AggregateResult> invoiceLineList = [SELECT InvoiceId__c, COUNT(Id) counter
                                                 FROM InvoiceLine__c 
                                                 WHERE InvoiceId__c =: invLineIdSet
                                                 GROUP BY InvoiceId__c];

//declare List to hold invoices with no lines & 
//loop the aggregate and add to the zero list where no lines 
    List<Invoice__c> zeroLineinvoiceIds = new List<Invoice__c>();
    for(AggregateResult ar : AggregateResult ){
        if(ar.get('counter') = 0){
            zeroLineinvoiceIds .add(ar.get('InvoiceId__c'));
        }
    }
    
if(zeroLineInvoiceIds.size() > 0 ) {
//now we have invoice id for no lines, lets find the Oppty
    List<Invoice__c> zeroLineInvoices = [SELECT Id, Opportunity FROM Invoice__c WHERE Id IN :zeroLineinvoiceIds ];
    
//decare list to hold Oppty Ids for Oppty search
//now loop the result set to get the Oppty Ids to be updated 
    List<Id> opptyIds = new List<Invoice__c>();
    for(Invoice__c inv : zeroLineInvoice) {
        opptyIds.add(inv.Opportunity);
    }

if(opptyIds.size() > 0 ) {
//get the actual Oppty to update
    List<Opportunity__c> oppList = [SELECT Id, CurrencyISOCode 
                                        FROM Opportunity__c 
                                        WHERE Id 
                                        IN: oppIdSet];

//for the Oppty list, update as required - but pass to a new list so we only do the DML if required.
    List<Opportunity> oppListToUpdate = new List<Opportunity>();
    if(!oppList.isEmpty()){
        for(Opportunity__c opp : oppList){
            if(opp.CurrencyISOCode != orgCurr){
                oppListToUpdate.add(new Opportunity__c( Id=opp.Id,
                                                        InvoiceTotal__c=0,
                                                        InvoiceGrossProfit__c=0,
                                                        InvoiceDiscountTotal__c=0));
            }
        }
    }

    if(oppListToUpdate.size()>0) {
        update oppListToUpdate;
    }
}
}
}

as a trigger, to me, that is more readable which is important when it comes to maintenance.  I would also put some description at the top to make it clear what the entire piece of code is.
And since we are talking triggers, I would be looking to move this level of complexity to a Trigger Handler, rather than leaving it in a Trigger.

regards
Andrew

P.s. all code is supplied uncompiled and as-is.  You will need to determine it suitablity for purpose for deployment in your org.

 
SidViciousSidVicious
I've finally sorted it out, thanks to your ideas.

It's my fault for not adding more context to the problem, and removing parts of the code in hopes of simplifying the question but which may have been confusing to the reader.

I'll do the @TestSetup for the Test Classes.

I've marked your answer as Best Answer.

 Thanks again1