3

I have written a very simple controller extension to show some data from a related list in a Visualforce pdf.

public with sharing class adviceRelList {
    public adviceRelList(ApexPages.StandardController standardController){       
        String quoteId = [SELECT Id FROM Quote WHERE Id =: ApexPages.currentPage().getParameters().get('id')].Id;
        String extrId = quoteId.left(15);
        List<Advies__c> adviceList = new List<Advies__c>([SELECT Specifiek_Advies__c, Id__c FROM Advies__c where Id__c =: extrId]);
    }
}

Here's the test class (probably all wrong):

@isTest(SeeAllData=true)
public class testAdviceRelList {
    public static testMethod void testAdviceRelList() {
        Advies__c testAdvies = new Advies__c();
        testAdvies.Specifiek_Advies__c = 'example';
        insert testAdvies;

        Test.startTest();
        ApexPages.StandardController sc = new ApexPages.StandardController(testAdvies);
        adviceRelList e = new adviceRelList(sc);

        PageReference pageRef = Page.offerteAdjNoDisNL;
        pageRef.getParameters().put('id', String.valueOf(testAdvies.Id__c));
        Test.setCurrentPage(pageRef);

        Test.stopTest();
    }
}

To me the issue seems to be the getParameters. I get

System.QueryException: List has no rows for assignment to SObject on Class.adviceRelList.: line 5, column 1
Class.testAdviceRelList.testAdviceRelList: line 8, column 1.

However when I simply put the getParameters in the adviceList SOQL query the test works fine. Unfortunately for some reason the page Id retrieved is not the same as the Id from the quote so I have to 'rewrite' it first.

1

Basti's answer addresses the surface issue (passing Advies__c record where you look to be expecting a Quote), but there is more trouble lurking beneath.

First, let's take a look at the first line of code in your controller extension

String quoteId = [SELECT Id FROM Quote WHERE Id =: ApexPages.currentPage().getParameters().get('id')].Id;

You're querying for the Id of a quote where the Id is equal to what you get from the page parameters, and then you fetch the Id of the returned record...assuming one is returned (which in your current case, it's not, which is the source of your error).

That's a very roundabout way to get the id. After all, you already have access to the Id via the page parameters. There is no need to query for it.

So, we can remove your query entirely, but your code still isn't very robust (if this came to me in a code review, I would immediately reject it).

What happens if you pass a null Id? What happens if you pass an Id for something that isn't a Quote?

Code that is more robust will be able to answer those questions. My first shot at improving your controller extension would be

public with sharing class adviceRelList {
    // Chances are you'll want to actually access/display the Advice records you're
    //   querying for.
    // To do that, you need a class property (or a plain 'ol class variable with
    //   a 'getter' method that you'd write yourself, but in most cases we can let
    //   Salesforce take care of providing the 'getter' and 'setter' method.
    // A 'class property' is simply a variable that has a get() and a set() method.
    // The "{get; set;} takes care of that.
    public List<Advies__c> adviceList {get;set;}

    public adviceRelList(ApexPages.StandardController standardController){
        // Page parameters are strings, but explicitly type-casting it as an Id
        //   will provide some benefits.
        // quoteId also needs to be an Id (instead of a string) for this.
        //Id quoteId = (Id)(ApexPages.currentPage().getParameters().get('id'));

        // I've commented out the line above because what you actually want to do
        //   is likely pull the Id from the standard controller, rather than from
        //   the page parameters.

        Id quoteId = (Id)standardController.getId();

        // Here is where we start to make the code more robust.
        // First, let's initialize adviceList so the visualforce page has something
        //   to work with.
        // An empty list is safer than a null (uninitialized) list
        adviceList = new List<Advies__c>();

        // Check to see if the Id is null or not, and if it's an Id for a quote.
        // This is where having quoteId as an Id is helpful, the Id class has a
        //   getSObjectType() method, and we can compare this against the SObjectType 
        //   for Quote.
        // You need to do the null check first, otherwise you can encounter a NullPointerException
        //   (null has no methods on it, so null.someMethod() results in an NPE.
        //   if quoteId is null, the if statement 'short-circuits' and we enter the
        //   if block without evaluating quoteId.getSObjectType(). No evaluation = no NPE)
        if(quoteId == null || quoteId.getSObjectType() == Quote.SObjectType){
            // If the Id is null or not for a quote, there's nothing more that we want
            //   to do (well, except maybe for making a page message to let people know
            //   what went wrong)
            ApexPages.Message myMsg = new ApexPages.Message(ApexPages.severity.WARNING, 'Related list couldn\'t be fetched because the Id was either null or not a Quote Id');
            ApexPages.currentPage.addMessage(myMsg);

            // By doing an explicit return here, we prevent the rest of the code from executing.
            return;
        }
        // 15 vs 18 character Id makes no difference in a query, so this line is not needed
        //String extrId = quoteId.left(15);

        // adviceList has already been declared, so we need to drop the 'List<Advies__c>'
        //   from the start of this line (else you'll get a variable redeclaration error)
        // We can also just simply use quoteId directly
        adviceList = new List<Advies__c>([SELECT Specifiek_Advies__c, Id__c FROM Advies__c where Id__c = :quoteId]);
    }
}

Your test class also has a few issues. The first of which is seeAllData=true. That's a dangerous annotation to use. It is required in a few very specific situations, but this is not one of them. Tests are better if they generate their own test data (that way, you aren't tempted to hard-code Ids in your tests, which will cause tests to fail in some environments. It also prevents your test from changing any real data).

I like to say that there are 3 phases of a unit test:

  1. Test setup
  2. Executing one, and only one, unit of code
  3. Verify results (using assertions)

Your test is missing part of phase 1 (by virtue of using seeAllData=true), and is missing phase 3 entirely.

For phase 1, test setup, you should be (at the very least) creating a quote record. Personally, I'd create two. Then, you'd insert those test quote records, and go on to create your test Advies__c records (again, at least one, but more is probably appropriate)

// Phase 1: test setup
// You don't _need_ to create this list, but doing so is a good habit to have.
// It allows you to gather all of your test records and insert them in one shot (as
//   opposed to one at a time), which becomes more important with more complicated
//   test setups.
List<Quote> testQuotes = new List<Quote>();

// I'll create two quotes
// The only data we need from the quote for our test is the quote Id.
// Of course, you may need to specify additional fields to satisfy system validation
//   rules (or custom validation rules that you've made).
// If you need to set fields, you can do it using key-value pairs passed to the
//   SObject constructor like Quote q = new Quote(OpportunityId = oppId, my_field__c = 'September');
// This may mean that you need to create/insert, say, an Opportunity, which in
//   turn may require an Account.
// It can be quite tedious to set up _all_ the prerequisite data that you need, but
//   one of the goals of a unit test is to control as much input as you can so that
//   the output is predictable.
testQuotes.add(new Quote());
testQuotes.add(new Quote());

insert testQuotes;

// Now, we make some Advies__c records
// After inserting, each quote in the testQuotes list will have an Id that we can use
List<Advies__c> adviceList = new List<Advies__c>();
// I'd recommend changing the Id__c field name to be Quote__c instead.
// Id__c will work, but it's very similar to the standard Id field (the field labels
//   are probably the same), which is a pain to deal with for page layouts.
// I like using prime numbers (yes, 2 is prime), so 2 records associated to one quote
//   and 3 associated to the other
adviceList.add(new Advies__c(Id__c = testQuotes[0].Id));
adviceList.add(new Advies__c(Id__c = testQuotes[0].Id));
adviceList.add(new Advies__c(Id__c = testQuotes[1].Id));
adviceList.add(new Advies__c(Id__c = testQuotes[1].Id));
adviceList.add(new Advies__c(Id__c = testQuotes[1].Id));

insert adviceList;

You already have phase 2 of the test. That would be

// This is phase 2 of unit testing.
// This is mostly fine as-is, though my preference is to only surround the thing being tested
//   in test.startTest()/test.stopTest() (that would be the `adviceRelList e = new adviceRelList(sc);`
//   line in this particular case)
// A few things need to be re-ordered
Test.startTest();

// Setting the page reference for the test, and equally as important, the page parameters,
//   needs to happen before you make an instance of your controller extension.
// In this test, you're testing the extension's constrcutor, which is run exactly when
// adviceRelList e = new adviceRelList(sc); is executed.
PageReference pageRef = Page.offerteAdjNoDisNL;
pageRef.getParameters().put('id', String.valueOf(testQuote[1].Id));
Test.setCurrentPage(pageRef);

// Pass in a quote record instead of an Advies__c record
ApexPages.StandardController sc = new ApexPages.StandardController(testQuotes[0]);
adviceRelList e = new adviceRelList(sc);

Test.stopTest();

Phase 3 is arguably the most important part of unit testing. After your code has run, how do you know that it did what you think it did?

The answer is 'by using assertions'. If you don't have any assertions, you're only doing a 'smoke test', one that makes sure that your code executes without blowing up. There are different approaches to testing which would change what you might assert in your test, but the approach I use (most of the time) is to look at the code being tested, and see what is being changed by that code. The things being changed are the things you need to assert.

In this case, there are two main things that should be asserted

  1. We don't have a page message (which would indicate the Id is either null or for the wrong object)
  2. The Advies__c records that we queried for are the ones that are associated to the Quote that we're working with (not ones associated to any other Quotes, like testQuotes[1]).

Phase 3 of testing might look like this

// Phase 3 of unit testing, assertions
// It is quite common that you'll need to perform some more queries to get records
//   (or updates to test records) from the database.
// We don't need to do that here, but just keep in mind the general pattern is
// 1) query for results 2) manipulate data to be in a convenient form 3) do the assertion

// Good idea to check that we don't have any page messages
System.assertEquals(true, pageRef.getMessages().isEmpty(), 'There shouldn\'t have been any page messages');

// Good to check that the number of Advies__c records we have matches our expectation.
// We could just say, "oh, yeah, I know we related 2 records to this quote", but
//   that's what's known as a "magic number".
// More often than not, it's hard to tell why a Magic number is what it is.
// It's better to derive the number that we expect through another means (which
//   also has the benefit of automatically updating if this test is updated to, say, associate
//   5 records to testQuotes[0])
List<Advies__c> expectedList = [SELECT Id FROM Advies__c WHERE Id__c = :testQuotes[0].Id];
System.assertEquals(expectedList.size(), e.adviceList.size(), 'Extension contained a different number of Advies__c records than expected');

// Finally, it's a good idea to make sure that the records truly are related to the
//  target quote (having the same number of records isn't good enough for me)
for(Advies__c advice :e.adviceList){
    // Between this assertion and the previous one, this is why I created
    //   multiple test quotes, and a different number of records related to them.
    // If the previous assertion failed because we got 3 records or 5 records
    //   that would give us a good idea of what went wrong (there would likely
    //   be an error in the query that was excluding the target quote id, or
    //   querying all Advies__c records).
    // Good assertions will give you an idea of where you need to look to fix things
    //   in the code that is being tested.
    System.assertEquals(testQuotes[0].Id, advice.Id__c, 'One of the Advies__c records pulled isn\'t related to the quote we expected it to be related to);
}

Combined, that should be a single, decent, unit test. One test is often not enough, however. Code coverage is a secondary concern (but an important one). If you find yourself lacking in code coverage, then you need to create more unit tests.

Generally speaking, a single unit test will cover a single input configuration. If you have an if(<condition>){ //code}else{ //other code}, you should only be executing one of those two branches in a single unit test. To cover both branches, you should have 2 test methods (one for each). Getting 100% coverage is not always possible (or practical), but you want to make sure you test what happens in more than just the 'happy path' where all input is exactly as you expect it to be.

In your case, some 'non happy-path' situations that you want to cover are:

  • What happens if a blank record is passed into your page?
  • What happens if you pass a record other than a Quote into your controller extension?

The first of those isn't a very realistic concern (though it can happen if you use your controller extension in some other apex class), but the second one is.

The more "what ifs" you test for, the more confident you can be that your code is "robust" (able to gracefully handle adverse situations) and does the correct thing. It also makes it harder for future changes to code (either your extension, or other code/objects that your extension may use) to break your existing code without someone noticing it.

| improve this answer | |
  • I can't thank you enough for this thorough explanation. I have yet to implement it but I just wanted to thank you beforehand. – Evert de Ruiter Oct 24 '17 at 17:38
  • @EvertdeRuiter Glad you found it useful . I'll admit that this one is among the longer answers I've given (and I think my answers are generally longer than average to begin with), and I probably spent too much time on it. I like trying to get to the root of the issue, and include extra information so that you (and others with similar issues) can know the why of the issue along with the what of the issue (and how to avoid it in the future). Again, it's nice to know that this extra effort isn't in vain. – Derek F Oct 24 '17 at 18:08
  • I implemented your fixes and got 100% coverage because of your answer so I'm more than thankful! And as you probably guessed I'm not the most experienced Salesforce 'developer' to put it lightly so the extra information helped me learn as well. – Evert de Ruiter Oct 24 '17 at 18:36
2

Your controller works with a Quote standard controller, but you are passing a list with Advies__c records to your Test controller.

Link your Advices to a Quote and passe this object to yout standard controller.

| improve this answer | |

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.