apex-enterprise-patterns / fflib-apex-common

Common Apex Library supporting Apex Enterprise Patterns and much more!
BSD 3-Clause "New" or "Revised" License
899 stars 514 forks source link

Disable Savepoint in tests #467

Closed pellmr closed 9 months ago

pellmr commented 10 months ago

Is your feature request related to a problem? Please describe. We want to use the Unit of work to help with setting up test data for functional/integration tests, but if the code being tested has a callout we receive a You have already created Savepoints. You cannot make callout after creating a Savepoint error.

Describe the solution you'd like Skip creating a savepoint if Test.isRunningTest() is true

daveespo commented 10 months ago

@pellmr -- can you provide a very concise repro case here? Something doesn't sound right -- we have UnitOfWork create data in our TestSetup method and then do callouts in our test methods (using Test.setMock first, of course) and have never seen the error you're reporting.

pellmr commented 10 months ago

Hmm...I'm probably just doing something wrong then. Here is a contrived example for simplicity.

Class

public without sharing class UowIssueDemo {
  public static void makePostCalloutPreWork() {
    Contact testContact = [SELECT id FROM Contact LIMIT 1];
    makePostCallout(testContact.Id);
  }

  @future(callout=true)
  public static void makePostCallout(Id contactId) {
    Contact c = [SELECT FirstName FROM Contact LIMIT 1];

    Http http = new Http();
    HttpRequest request = new HttpRequest();
    request.setEndpoint('https://fflibuow.free.beeceptor.com/contact');
    request.setMethod('POST');
    request.setHeader('Content-Type', 'application/json;charset=UTF-8');
    request.setBody('{"name": "' + c.firstname + '" }');
    HttpResponse response = http.send(request);

    if (response.getStatusCode() != 201) {
      System.debug(
        'The status code returned was not expected: ' +
          response.getStatusCode() +
          ' ' +
          response.getStatus()
      );
    } else {
      System.debug(response.getBody());
    }
  }
}

Test Class

@isTest
public with sharing class UowIssueDemoTest {
  @isTest
  static void testPostCallout() {
    //Arrange
    fflib_SObjectUnitOfWork uow = new fflib_SObjectUnitOfWork(
      new List<SObjectType>{ Contact.SObjectType }
    );
    uow.registerNew(new Contact(FirstName = 'DemoGuy', LastName = 'Firebird');
    uow.commitWork();

    Test.setMock(HttpCalloutMock.class, new UowIssueHttpCalloutMock());

    //Act
    Test.startTest();
    UowIssueDemo.makePostCalloutPreWork();
    Test.stopTest();

    //Assert
    System.assertEquals(Limits.getFutureCalls(), 1, '1 future should be called');
  }
}

Error UowIssueDemoTest.testPostCallout UowIssueDemo Fail 92% System.CalloutException: You have already created Savepoints. You cannot make callout after creating a Savepoint

ImJohnMDaniel commented 9 months ago

So, if you are using the UnitOfWork from the Apex Common framework are you also using the Selector pattern?

If you are using the Selector pattern, then are you using Apex Mocks in the unit tests? Doing so would solve this issue.

daveespo commented 9 months ago

This appears to be an undocumented limitation of @future and Queueable methods with regard to Savepoints. I can indeed repro the problem as reported. And if I replace SOUOW with a straight DML insert of a new Contact, the test completes (it fails for another reason -- the assertion to test the number of Future calls never increments -- but that's not relevant here)

We are not going to change the SOUOW behavior in unit test context. The risk of masking failures in production code execution context by not using the same code path in unit test context far outweighs the impact of this corner case.

As an aside: future methods have long fallen out of favor -- Salesforce introduced Queueables a decade ago and has steadily increasing the flexibility and feature set of Queueables -- I am not aware of any use case of @future that can't be done equivalently with Queueable. But again, Queueables won't save the day here; they are subject to the same limitation.

The only workaround I can offer is what I posted in my first comment. If you do the SOUOW commitWork in a @TestSetup method, that is compatible with using an async/callout in a test method.