1

I have a scenario like this,

I have created Opportunities in a Test setup and tried to access it in a test method to clone that record like,

 testSetup {
    // Create a list of Opportunity Records.
    List<Opportunity> lstOpportunities = new List<Opportunity>();

    // Create a 11111111 Opportunity Record.
    Opportunity objOpportunity11111111 = new Opportunity(
            Name = 'Test Opportunity',
            AccountId = objAccount.Id,
            StageName = 'Closed Won',
            CloseDate = System.today(),
            RecordTypeId = '11111111'
    );

    // Create a 222222 Sales Opportunity Record
    Opportunity objOpportunity222222 = new Opportunity(
            Name = 'Test Opportunity 1',
            AccountId = objAccount.Id,
            StageName = 'Closed Won',
            CloseDate = System.today(),
            RecordTypeId = '222222'
    );

    lstOpportunities.add(objOpportunity11111111);
    lstOpportunities.add(objOpportunity222222);

    insert lstOpportunities;
}

I have created a method to check the 11111111 record type Opportunity cloning functionality as follows,

@IsTest
static void cloneRecordTest() {
    Id recordTypeId = '11111111';
    List<Opportunity> lstOpportunityBefore = [SELECT Id, Name FROM Opportunity WHERE RecordTypeId = :recordTypeId];

    Opportunity objOpportunityOne = new Opportunity();
    objOpportunityOne.Name = 'Test Opportunity One';
    objOpportunityOne.StageName = 'Closed Won';
    objOpportunityOne.CloseDate = System.today();
    objOpportunityOne.RecordTypeId = '11111111'

    String jsonObject = JSON.serialize(objOpportunityOne);
    Test.startTest();

    OpportunityEditForm_Ctrl.cloneRecord(jsonObject, lstOpportunityBefore[0].Id);
    Test.stopTest();
    List<Opportunity> lstOpportunityAfter = [SELECT Id FROM Opportunity WHERE RecordTypeId = :recordTypeId];
    System.assertEquals(1, lstOpportunityBefore.size());
    System.assertEquals(2, lstOpportunityAfter.size());
}

Is it really needed to check whether the lstOpportunityBefore list has a record (lstOpportunityBefore.size()>0) before access to call the method.

I would say we don't need to check because if there are no records then the method will fail because the assertion will fail. As we have created the test data, so we don't need to check it before access the elements of a list.

Appreciate the Experts comment.

  • Its not needed, also It might create a problem if you have testSetup inserting more opportunities with the same criteria – rahul gawale Jul 2 at 4:37
3

It depends on who you ask.

Since you asked me, here's my answer.

No, it's not required to write assertions for assumptions that must be true in order for the test to pass.

In this case, the lstOpportunityBefore should always be the correct size. Similarly, the lstOpportunityAfter should always be the correct size if you get this far in the code.

My rule of thumb is that if a system exception will be thrown if my assumptions are wrong, I will not write an assertion for that condition.

I leverage the fact that the system can throw QueryException, ListException, NullPointerException, etc if a bug occurs somewhere, and that's usually enough for me to figure out how to fix it.

If you feel you need it, feel free to use them. For code that may or may not create too many/too few records, etc, feel free to check the list to make sure they're the right size.

However, if you can assume a certain condition, it's okay to skip an assertion for that condition. If your assumptions are somehow broken, the system will let you know via an exception.

Let's work on a trivial example.

Integer x = 10 / 5;
System.assertEquals(2, x);

Would you write this assertion? No. Why? Because we can assume that math will always work. While this is trivial, making assumptions like this can reduce the amount of code we have to write, decreasing both development and deployment times.

Take advantage of your assumptions whenever you can. Just always carefully consider your algorithm when you're done to make sure you're not missing any edge cases.

| improve this answer | |
  • Another great answer from @sfdxfox. I've gotten into the habit of adding a test method in test Classes that does nothing except verify that test records setup with an @testSetup annotation. It's intended to protect me from a new required field or validation rule on an Object. In fact my scaffolding snippet for any test Class includes two method by default. setupTestData() and testSetupData() – John Thompson Jul 2 at 15:25
3

I generally agree with sfdcfox's answer and for this specific case wouldn't have an assert.

I would, however, point out that "your" assumptions are best captured in code comments to inform maintainers of your code because "your" assumptions will become (implicitly) "their" assumptions and they may not be aware of them.

A comment like:

// We know only one opportunity with this record type is created in the test setup

helps speed up a correction to this specific test if someone later changes the test setup in support of other tests in the same test class.

Even with sfdcfox's trivial example:

Integer x = 10 / 5;
...
System.assertEquals(2, x);

I would say having the assert might make sense if the declaration of the value and the usages are significantly separated in the code and the usages are non-trivial.

Basically, if it is likely to save a future you (or your replacement) time in understanding the original assumptions I would say add the assertion but certainly always add the comment.

| improve this answer | |
0

In my eyes, the thing that you care about here isn't that you started with 1 Opportunity and ended with 2 Opportunities. Instead, what you care about is that you have 1 more Opportunity after executing the code being tested (compared to before the code being tested was executed).

What you have right now

System.assertEquals(1, lstOpportunityBefore.size());
System.assertEquals(2, lstOpportunityAfter.size());

is one way to do that.

The approach I use is more flexible/robust, I think.

// Assert that the difference between collection sizes is a certain amount
System.assertEquals(1, lstOpportunityAfter.size() - lstOpportunityBefore.size(), 'There should have been 1 new Opportunity created');

That third parameter is optional, but I find that having a brief failure message really helps in not only locating the failed assertion, it also helps get me into the proper mindset for debugging (I'm looking specifically for issue X).

This "assert the difference" approach also relies less on "Magic Numbers" (one definition of which is numbers that appear in code with little/no explanation for why they are the value they are).

P.S. Don't forget to have a test method that makes assertions that your cloned record's field values are the same as your original record.

| improve this answer | |
  • I agree with @Derek F. Your answer saws that how we can improve the test method, but my question here is even simple, whether we need to check lstOpportunityBefore List contains an element or not when calling the method, because I am the person who creates the test data, so there won't be an issue while cloning the record and if there will be no record in the list then the method will be failing according to the Assertion. – Hariprasath Jul 3 at 4:07

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.