Conversation
Into SObjectFactory and several helper classes. Classes now adhere to SRP, and are in general better architected.
To various helper classes.
|
WalkthroughThe changes primarily involve a significant refactoring of the Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public class AccountDefaults implements SObjectFactory.FieldDefaults { | ||
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Account.Name => 'Test Account' | ||
| }; | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Account.Name => 'Test Account' | ||
| }; | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public class ContactDefaults implements SObjectFactory.FieldDefaults { | ||
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Contact.FirstName => 'First', | ||
| Contact.LastName => 'Last' | ||
| }; | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Contact.FirstName => 'First', | ||
| Contact.LastName => 'Last' | ||
| }; | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public class OpportunityDefaults implements SObjectFactory.FieldDefaults { | ||
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Opportunity.Name => 'Test Opportunity', | ||
| Opportunity.StageName => 'Closed Won', | ||
| Opportunity.CloseDate => System.today() | ||
| }; | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Opportunity.Name => 'Test Opportunity', | ||
| Opportunity.StageName => 'Closed Won', | ||
| Opportunity.CloseDate => System.today() | ||
| }; | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public class CaseDefaults implements SObjectFactory.FieldDefaults { | ||
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Case.Subject => 'Test Case' | ||
| }; | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
| public Map<Schema.SObjectField, Object> getFieldDefaults() { | ||
| return new Map<Schema.SObjectField, Object>{ | ||
| Case.Subject => 'Test Case' | ||
| }; | ||
| } |
Check warning
Code scanning / PMD
Missing ApexDoc comment Warning test
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (18)
- .idea/dictionaries/project.xml (1)
- .idea/vcs.xml (1)
- force-app/main/default/classes/test utilities/PermissionsHelper.cls (1)
- force-app/main/default/classes/test utilities/PermissionsHelper.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/SObjectFactory.cls (1)
- force-app/main/default/classes/test utilities/SObjectFactory.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/SObjectFactoryDefaults.cls (1)
- force-app/main/default/classes/test utilities/SObjectFactoryDefaults.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/SObjectFactoryHelper.cls (1)
- force-app/main/default/classes/test utilities/SObjectFactoryHelper.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/SObjectFactoryTests.cls (1)
- force-app/main/default/classes/test utilities/SObjectFactoryTests.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/SomeDefaultsClass.cls (1)
- force-app/main/default/classes/test utilities/SomeDefaultsClass.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/UserFactory.cls (1)
- force-app/main/default/classes/test utilities/UserFactory.cls-meta.xml (1)
- force-app/main/default/classes/test utilities/UserFactoryHelper.cls (1)
- force-app/main/default/classes/test utilities/UserFactoryHelper.cls-meta.xml (1)
Files ignored due to filter (1)
- IlluminatedCloud/Apexkit/OfflineSymbolTable.zip
Files skipped from review due to trivial changes (9)
- .idea/dictionaries/project.xml
- .idea/vcs.xml
- force-app/main/default/classes/test utilities/PermissionsHelper.cls-meta.xml
- force-app/main/default/classes/test utilities/SObjectFactory.cls-meta.xml
- force-app/main/default/classes/test utilities/SObjectFactoryHelper.cls-meta.xml
- force-app/main/default/classes/test utilities/SObjectFactoryTests.cls-meta.xml
- force-app/main/default/classes/test utilities/SomeDefaultsClass.cls-meta.xml
- force-app/main/default/classes/test utilities/UserFactory.cls-meta.xml
- force-app/main/default/classes/test utilities/UserFactoryHelper.cls-meta.xml
Review comments generated (23)
- Review: 8
- LGTM: 15
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| /** | ||
| * @description Tests for test utilities. | ||
| */ | ||
| @IsTest | ||
| @SuppressWarnings('PMD.AvoidDebugStatements') | ||
| private class SObjectFactoryTests { | ||
| @IsTest | ||
| static void profilingTestCreate10IndividualSObjectAccountNoInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| for (Integer index = 0; index < 10; index++) { | ||
| SObjectFactory.createSObject(new Account()); | ||
| } | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create 10 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create 10 accounts to be less than 75ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreate10IndividualSObjectAccountNoInsertWithCustomDefaultsPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| for (Integer index = 0; index < 10; index++) { | ||
| SObjectFactory.createSObject(new Account(), 'SomeDefaultsClass'); | ||
| } | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create 10 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create 10 accounts to be less than 75ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreate10IndividualSObjectAccountWithInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| for (Integer index = 0; index < 10; index++) { | ||
| SObjectFactory.createSObject(new Account(), true); | ||
| } | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create & insert 10 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create 10 accounts to be less than 1500ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreateListOf200SObjectsNoInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| SObjectFactory.createSObjects(new Account(), 200); | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create a list of 200 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create a list of 200 accounts to be less than 150ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreateListOf200SObjectsWithInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| SObjectFactory.createSObjects(new Account(), 200, true); | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create and insert a list of 200 accounts: ' + | ||
| (stopTime - startTime) + | ||
| 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create and insert a list of 200 accounts to be less than 1500ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectsWith200AccountsNoCustomDefaultsPositive() { | ||
| Test.startTest(); | ||
| SObjectFactory.createSObjects(new Account(), 200, null, true); | ||
| Test.stopTest(); | ||
| Account[] checkAccounts = [SELECT Id FROM Account]; | ||
| Assert.areEqual( | ||
| 200, | ||
| checkAccounts.size(), | ||
| 'expected 200 accounts to be created' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectsWith200AccountsWithCustomDefaultsPositive() { | ||
| Test.startTest(); | ||
| SObjectFactory.createSObjects( | ||
| new Account(), | ||
| 200, | ||
| 'SomeDefaultsClass', | ||
| true | ||
| ); | ||
| Test.stopTest(); | ||
| Account[] checkAccounts = [SELECT Id FROM Account]; | ||
| Assert.areEqual( | ||
| 200, | ||
| checkAccounts.size(), | ||
| 'expected 200 accounts to be created' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectsWith200AccountsWithCustomDefaultsProvesUniqueIdsPositive() { | ||
| Test.startTest(); | ||
| List<Account> checkAccounts = (List<Account>) SObjectFactory.createSObjects( | ||
| new Account(), | ||
| 200, | ||
| 'SomeDefaultsClass', | ||
| false | ||
| ); | ||
| Test.stopTest(); | ||
| Set<Id> fakeIds = new Set<Id>(); | ||
| for (Account accountToCheck : checkAccounts) { | ||
| fakeIds.add(accountToCheck.Id); | ||
| } | ||
| Assert.areEqual( | ||
| 200, | ||
| fakeIds.size(), | ||
| 'Expected a set of unique ids to be the same size as the requested number of objects to be created' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectWithAccountAndInsertPositive() { | ||
| Test.startTest(); | ||
| SObjectFactory.createSObject(new Account(), 'SomeDefaultsClass', true); | ||
| Test.stopTest(); | ||
| Account[] checkAccounts = [SELECT Id FROM Account]; | ||
| Assert.areEqual( | ||
| 1, | ||
| checkAccounts.size(), | ||
| 'expected 200 accounts to be created' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests are well written and cover a variety of scenarios. However, there is a minor issue in line 161 where the assertion message does not match the test case. The test is checking if one account was created but the message says "expected 200 accounts to be created". This could lead to confusion when debugging failed tests.
- 'expected 200 accounts to be created'
+ 'expected 1 account to be created'Also, consider using more descriptive test method names that clearly state what each test is doing. For example, instead of profilingTestCreate10IndividualSObjectAccountNoInsertPositive, you could use shouldCreate10AccountsWithoutInsertingInExpectedTime. This makes it easier for other developers to understand what each test is supposed to do.
| /** | ||
| * @description A factory class for generating Users during Unit Tests | ||
| * While this class has generic user-building methods, the bulk of this | ||
| * class is dedicated to building persona-specific users | ||
| */ | ||
| @IsTest | ||
| public class UserFactory { | ||
| /// Persona based factory methods | ||
|
|
||
| /** | ||
| * @description Creates a user with the Minimum Access Profile | ||
| * @param forceInsert Should this code insert the user? | ||
| * @return `User` | ||
| */ | ||
| public static User createMinAccessPersonaUser(Boolean forceInsert) { | ||
| return createTestUser( | ||
| UserFactoryHelper.fetchProfileIdByName('Minimum Access - Salesforce'), | ||
| forceInsert | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @description creates a Marketing Persona user | ||
| * @param forceInsert Boolean should this user be inserted? | ||
| * @return User created marketing user | ||
| */ | ||
| public static User createMarketingPersonaUser(Boolean forceInsert) { | ||
| User createdUser = createTestUser('Marketing User', forceInsert); | ||
| return createdUser; | ||
| } | ||
|
|
||
| /// Generic user factory methods | ||
|
|
||
| /** | ||
| * @description creates a test user. Useful for permissions testing | ||
| * @param profileId Profile Id to use when creating a user. | ||
| * @param forceInsert Boolean, should this code insert the user? | ||
| * @return `User` | ||
| */ | ||
| public static User createTestUser(Id profileId, Boolean forceInsert) { | ||
| User userToCreate = new User( | ||
| ProfileId = profileId, | ||
| LastName = 'last', | ||
| Email = 'Testuser@test.example.com', | ||
| Username = 'Testuser@test.example.com' + Crypto.getRandomInteger(), | ||
| CompanyName = 'TEST', | ||
| Title = 'title', | ||
| Alias = 'alias', | ||
| TimeZoneSidKey = 'America/Los_Angeles', | ||
| EmailEncodingKey = 'UTF-8', | ||
| LanguageLocaleKey = 'en_US', | ||
| LocaleSidKey = 'en_US' | ||
| ); | ||
| UserFactoryHelper.forceInsert(userToCreate, forceInsert); | ||
| return userToCreate; | ||
| } | ||
|
|
||
| /** | ||
| * @description Creates a test user with a given profile. | ||
| * @param profileName Name of the profile to create the user with. | ||
| * @param forceInsert Should this code insert the created user? | ||
| * @return `User` | ||
| */ | ||
| public static User createTestUser(String profileName, Boolean forceInsert) { | ||
| return createTestUser( | ||
| UserFactoryHelper.fetchProfileIdByName(profileName), | ||
| forceInsert | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code looks good overall, but there's a potential issue with the email generation in line 45. The Crypto.getRandomInteger() method can return negative numbers, which could lead to invalid email addresses. Consider using Math.abs(Crypto.getRandomInteger()) instead to ensure that the number is always positive.
- Username = 'Testuser@test.example.com' + Crypto.getRandomInteger(),
+ Username = 'Testuser@test.example.com' + Math.abs(Crypto.getRandomInteger()),Also, consider adding error handling for when fetchProfileIdByName returns null (lines 17 and 66). If the profile name provided does not exist, this method will return null and cause a NullPointerException when trying to create a User.
- UserFactoryHelper.fetchProfileIdByName('Minimum Access - Salesforce'),
+ UserFactoryHelper.fetchProfileIdByName('Minimum Access - Salesforce') != null ? UserFactoryHelper.fetchProfileIdByName('Minimum Access - Salesforce') : someDefaultProfileId,Replace someDefaultProfileId with an actual default Profile Id. Apply similar changes to line 66.
| @IsTest | ||
| public with sharing class SObjectFactory { | ||
| /** |
There was a problem hiding this comment.
The @IsTest annotation is used for test classes and methods. It seems like this class is not a test class but a utility class for creating test data. Consider removing the @IsTest annotation.
- @IsTest|
|
||
| /** | ||
| * @description Creates an SObject with the given prototype, using defaults from the specified class, and optionally inserts it into the database. | ||
| * | ||
| * @param prototype The prototype SObject to be created. | ||
| * @param usingDefaultsClassName The name of the class providing default values for the SObject. | ||
| * @param forceInsert Indicates whether to insert the SObject into the database. | ||
| * | ||
| * @return The created SObject. | ||
| */ | ||
| public static SObject createSObject( | ||
| SObject prototype, | ||
| String usingDefaultsClassName, | ||
| Boolean forceInsert | ||
| ) { | ||
| prototype = internalCreateSObject(prototype, usingDefaultsClassName); | ||
| if (forceInsert) { | ||
| Database.insert(prototype, AccessLevel.SYSTEM_MODE); | ||
| } else { | ||
| prototype.Id = IdFactory.get(prototype); | ||
| } | ||
| return prototype; | ||
| } |
There was a problem hiding this comment.
In line 87, you are using Database.insert(prototype, AccessLevel.SYSTEM_MODE);. This will bypass all custom security settings such as object-level, field-level, and record-level access permissions. Be sure that this is what you want to do. If not, consider using insert prototype; instead.
- Database.insert(prototype, AccessLevel.SYSTEM_MODE);
+ insert prototype;|
|
||
| /** | ||
| * @description Creates a list of SObjects based on a prototype, count, defaults class, and force insert flag. | ||
| * | ||
| * @param prototype The prototype SObject to clone. | ||
| * @param count The number of SObjects to create. | ||
| * @param usingDefaultsClassName The name of the defaults class to use. | ||
| * @param forceInsert Whether to force insert the created SObjects. | ||
| * | ||
| * @return A list of created SObjects. | ||
| */ | ||
| @SuppressWarnings('PMD.ExcessiveParameterList') | ||
| public static List<SObject> createSObjects( | ||
| SObject prototype, | ||
| Integer count, | ||
| String usingDefaultsClassName, | ||
| Boolean forceInsert | ||
| ) { | ||
| List<SObject> createdSObjects = new List<SObject>(); | ||
| SObject constructedFromPrototype = internalCreateSObject( | ||
| prototype, | ||
| usingDefaultsClassName | ||
| ); | ||
| for ( | ||
| Integer iterationCounter = 0; iterationCounter < count; iterationCounter++ | ||
| ) { | ||
| SObject clonedSObject = constructedFromPrototype.clone(false, true); | ||
| createdSObjects.add( | ||
| SObjectFactoryHelper.mutateCloneToRespectNameAndAutonumberRules( | ||
| clonedSObject, | ||
| !forceInsert, | ||
| iterationCounter | ||
| ) | ||
| ); | ||
| } | ||
| SObjectFactoryHelper.insertIfForced(createdSObjects, forceInsert); | ||
| return createdSObjects; | ||
| } |
There was a problem hiding this comment.
In line 194, you are using SObjectFactoryHelper.insertIfForced(createdSObjects, forceInsert);. This method might be bypassing all custom security settings such as object-level, field-level, and record-level access permissions. Be sure that this is what you want to do. If not, consider using a simple insert createdSObjects; when forceInsert is true.
- SObjectFactoryHelper.insertIfForced(createdSObjects, forceInsert);
+ if (forceInsert) {
+ insert createdSObjects;
+ }| /** | ||
| * @description A test-only helper for manipulating users' permissions, permission sets and permission set groups | ||
| * | ||
| * Note: This class does *no* error handling. Because it's used in the setup and preparation of test data, | ||
| * it's on the developer to ensure the methods are called with existing, valid data like permission set names. | ||
| * If, for instance, a developer fat-fingers a permission set name, the query on ~ line 74 will fail, and throw | ||
| * an exception. | ||
| */ | ||
|
|
||
| @IsTest | ||
| public class PermissionsHelper { | ||
| /** | ||
| * @description creates and inserts a permission set assignment record | ||
| * @param userToAssignTo User the user whom the permission set will be applied to | ||
| * @param permSetId Id The Id of the permission set to assign to the user. | ||
| */ | ||
| public static void assignPermissionSetToUser( | ||
| User userToAssignTo, | ||
| Id permSetId | ||
| ) { | ||
| PermissionSetAssignment permissionSetAssignment = new PermissionSetAssignment( | ||
| AssigneeId = userToAssignTo.Id, | ||
| PermissionSetId = permSetId | ||
| ); | ||
| insert permissionSetAssignment; | ||
| } | ||
|
|
||
| /** | ||
| * @description Assigns a permission set to a given user. | ||
| * @param userToAssignTo User to assign the permission set to. | ||
| * @param permSetName String name of the permission set. | ||
| */ | ||
| public static void assignPermSetToUser( | ||
| User userToAssignTo, | ||
| String permSetName | ||
| ) { | ||
| assignPermissionSetToUser( | ||
| userToAssignTo, | ||
| UserFactoryHelper.fetchPermissionSetIdByName(permSetName) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @description Generates a test permission set record - no permissions are added to it | ||
| * @param permSetName String what to call your perm set | ||
| * @param doInsert Boolean true if you want this to insert your perm set record. | ||
| * @return PermissionSet the created permission set. | ||
| */ | ||
| public static PermissionSet createPermissionSet( | ||
| String permSetName, | ||
| Boolean doInsert | ||
| ) { | ||
| PermissionSet newPermSet = new PermissionSet( | ||
| Name = permSetName, | ||
| Label = 'Test Permission Set' | ||
| ); | ||
| if (doInsert) { | ||
| insert newPermSet; | ||
| } | ||
| return newPermSet; | ||
| } | ||
|
|
||
| /** | ||
| * @description Enables a custom permission using a permission set | ||
| * @param permissionName String name of the custom permission you want created | ||
| * @param forUserId Id user to assign the custom permission to. | ||
| */ | ||
| public static void enableCustomPermission( | ||
| String permissionName, | ||
| Id forUserId | ||
| ) { | ||
| PermissionSet permSet = createPermissionSet('TestPermSet', true); | ||
|
|
||
| Id customPermissionId = [ | ||
| SELECT Id | ||
| FROM CustomPermission | ||
| WHERE DeveloperName = :permissionName | ||
| WITH SYSTEM_MODE | ||
| LIMIT 1 | ||
| ] | ||
| .Id; | ||
|
|
||
| SetupEntityAccess permSetPermission = new SetupEntityAccess( | ||
| ParentId = permSet.Id, | ||
| SetupEntityId = customPermissionId | ||
| ); | ||
|
|
||
| PermissionSetAssignment permSetAssignment = new PermissionSetAssignment( | ||
| AssigneeId = forUserId, | ||
| PermissionSetId = permSet.Id | ||
| ); | ||
|
|
||
| insert new List<SObject>{ permSetPermission, permSetAssignment }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The code looks good overall, but there's a potential issue with the enableCustomPermission method. If the custom permission with the provided name doesn't exist, the query on line 74 will return no results and an attempt to access the .Id property on line 81 will result in a NullPointerException. To avoid this, you should check if the query returned any results before trying to access the Id.
- Id customPermissionId = [
- SELECT Id
- FROM CustomPermission
- WHERE DeveloperName = :permissionName
- WITH SYSTEM_MODE
- LIMIT 1
- ]
- .Id;
+ List<CustomPermission> permissions = [
+ SELECT Id
+ FROM CustomPermission
+ WHERE DeveloperName = :permissionName
+ WITH SYSTEM_MODE
+ LIMIT 1
+ ];
+
+ if (permissions.isEmpty()) {
+ throw new CustomPermissionNotFoundException('Custom permission ' + permissionName + ' not found');
+ }
+
+ Id customPermissionId = permissions[0].Id;In this fix, I've added a check for an empty list after the query. If the list is empty, it throws a CustomPermissionNotFoundException, which is a custom exception that you would need to define. This way, the error message can be more specific about what went wrong.
| public static Id fetchProfileIdByName(String profileName) { | ||
| Profile requestedProfile = [ | ||
| SELECT Id | ||
| FROM Profile | ||
| WHERE Name = :profileName | ||
| WITH SYSTEM_MODE | ||
| LIMIT 1 | ||
| ]; | ||
| return requestedProfile.Id; | ||
| } |
There was a problem hiding this comment.
The method fetchProfileIdByName could potentially throw a QueryException if the profile name provided does not exist in the system. It would be beneficial to handle this exception and provide a more meaningful error message to the developer.
- public static Id fetchProfileIdByName(String profileName) {
- Profile requestedProfile = [
- SELECT Id
- FROM Profile
- WHERE Name = :profileName
- WITH SYSTEM_MODE
- LIMIT 1
- ];
- return requestedProfile.Id;
- }
+ public static Id fetchProfileIdByName(String profileName) {
+ try {
+ Profile requestedProfile = [
+ SELECT Id
+ FROM Profile
+ WHERE Name = :profileName
+ WITH SYSTEM_MODE
+ LIMIT 1
+ ];
+ return requestedProfile.Id;
+ } catch (QueryException e) {
+ throw new MyCustomException('Profile with name ' + profileName + ' not found.');
+ }
+ }| public static Id fetchPermissionSetIdByName(String permissionSetName) { | ||
| Id permSetId = [ | ||
| SELECT Id | ||
| FROM PermissionSet | ||
| WHERE Name = :permissionSetName | ||
| WITH SYSTEM_MODE | ||
| LIMIT 1 | ||
| ] | ||
| .Id; | ||
| return permSetId; |
There was a problem hiding this comment.
Similar to the previous comment, the method fetchPermissionSetIdByName could also throw a QueryException if the permission set name provided does not exist in the system. Consider handling this exception and providing a more meaningful error message to the developer.
- public static Id fetchPermissionSetIdByName(String permissionSetName) {
- Id permSetId = [
- SELECT Id
- FROM PermissionSet
- WHERE Name = :permissionSetName
- WITH SYSTEM_MODE
- LIMIT 1
- ]
- .Id;
- return permSetId;
- }
+ public static Id fetchPermissionSetIdByName(String permissionSetName) {
+ try {
+ Id permSetId = [
+ SELECT Id
+ FROM PermissionSet
+ WHERE Name = :permissionSetName
+ WITH SYSTEM_MODE
+ LIMIT 1
+ ]
+ .Id;
+ return permSetId;
+ } catch (QueryException e) {
+ throw new MyCustomException('Permission Set with name ' + permissionSetName + ' not found.');
+ }
+ }To various helper classes.
There was a problem hiding this comment.
Files selected (3)
- force-app/main/default/classes/test utilities/SObjectFactoryDefaults.cls (1)
- force-app/main/default/classes/test utilities/SObjectFactoryTests.cls (1)
- force-app/main/default/classes/test utilities/UserFactory.cls (1)
Review comments generated (3)
- Review: 2
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| /** | ||
| * @description A factory class for generating Users during Unit Tests | ||
| * While this class has generic user-building methods, the bulk of this | ||
| * class is dedicated to building persona-specific users | ||
| */ | ||
| @IsTest | ||
| public class UserFactory { | ||
| /// Persona based factory methods | ||
|
|
||
| /** | ||
| * @description Creates a user with the Minimum Access Profile | ||
| * @param forceInsert Should this code insert the user? | ||
| * @return `User` | ||
| */ | ||
| public static User createMinAccessPersonaUser(Boolean forceInsert) { | ||
| return createTestUser( | ||
| UserFactoryHelper.fetchProfileIdByName('Minimum Access - Salesforce'), | ||
| forceInsert | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @description creates a Marketing Persona user | ||
| * @param forceInsert Boolean should this user be inserted? | ||
| * @return User created marketing user | ||
| */ | ||
| public static User createMarketingPersonaUser(Boolean forceInsert) { | ||
| User createdUser = createTestUser('Marketing User', forceInsert); | ||
| return createdUser; | ||
| } | ||
|
|
||
| /// Generic user factory methods | ||
|
|
||
| /** | ||
| * @description creates a test user. Useful for permissions testing | ||
| * @param profileId Profile Id to use when creating a user. | ||
| * @param forceInsert Boolean, should this code insert the user? | ||
| * @return `User` | ||
| */ | ||
| public static User createTestUser(Id profileId, Boolean forceInsert) { | ||
| User userToCreate = new User( | ||
| ProfileId = profileId, | ||
| LastName = 'last', | ||
| Email = 'Testuser@test.example.com', | ||
| Username = 'Testuser@test.example.com' + | ||
| Math.abs(Crypto.getRandomInteger()), | ||
| CompanyName = 'TEST', | ||
| Title = 'title', | ||
| Alias = 'alias', | ||
| TimeZoneSidKey = 'America/Los_Angeles', | ||
| EmailEncodingKey = 'UTF-8', | ||
| LanguageLocaleKey = 'en_US', | ||
| LocaleSidKey = 'en_US' | ||
| ); | ||
| UserFactoryHelper.forceInsert(userToCreate, forceInsert); | ||
| return userToCreate; | ||
| } | ||
|
|
||
| /** | ||
| * @description Creates a test user with a given profile. | ||
| * @param profileName Name of the profile to create the user with. | ||
| * @param forceInsert Should this code insert the created user? | ||
| * @return `User` | ||
| */ | ||
| public static User createTestUser(String profileName, Boolean forceInsert) { | ||
| return createTestUser( | ||
| UserFactoryHelper.fetchProfileIdByName(profileName), | ||
| forceInsert | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The UserFactory class seems well-structured and follows the Single Responsibility Principle (SRP). However, there's a potential issue with the email address generation in line 45. If two users are created at the same time, they could end up with the same email address, which would cause an error. To avoid this, consider appending a unique identifier like the current timestamp to the email address.
- Email = 'Testuser@test.example.com',
+ Email = 'Testuser' + DateTime.now().getTime() + '@test.example.com',Also, it's not clear what happens if the forceInsert parameter is false. The UserFactoryHelper.forceInsert(userToCreate, forceInsert); method call suggests that the user might not be inserted into the database, but the method still returns the userToCreate object. This could lead to confusion if the caller expects the returned user to exist in the database. Consider throwing an exception or returning null when forceInsert is false and the insert operation is skipped.
| /** | ||
| * @description Tests for test utilities. | ||
| */ | ||
| @IsTest | ||
| @SuppressWarnings('PMD.AvoidDebugStatements') | ||
| private class SObjectFactoryTests { | ||
| @IsTest | ||
| static void profilingTestCreate10IndividualSObjectAccountNoInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| for (Integer index = 0; index < 10; index++) { | ||
| SObjectFactory.createSObject(new Account()); | ||
| } | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create 10 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create 10 accounts to be less than 75ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreate10IndividualSObjectAccountNoInsertWithCustomDefaultsPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| for (Integer index = 0; index < 10; index++) { | ||
| SObjectFactory.createSObject(new Account(), 'SomeDefaultsClass'); | ||
| } | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create 10 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create 10 accounts to be less than 75ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreate10IndividualSObjectAccountWithInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| for (Integer index = 0; index < 10; index++) { | ||
| SObjectFactory.createSObject(new Account(), true); | ||
| } | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create & insert 10 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create 10 accounts to be less than 1500ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreateListOf200SObjectsNoInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| SObjectFactory.createSObjects(new Account(), 200); | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create a list of 200 accounts: ' + (stopTime - startTime) + 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create a list of 200 accounts to be less than 150ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void profilingTestCreateListOf200SObjectsWithInsertPositive() { | ||
| Test.startTest(); | ||
| Long startTime = Datetime.now().getTime(); | ||
| SObjectFactory.createSObjects(new Account(), 200, true); | ||
| Long stopTime = Datetime.now().getTime(); | ||
| Test.stopTest(); | ||
| System.debug( | ||
| LoggingLevel.DEBUG, | ||
| 'Time to create and insert a list of 200 accounts: ' + | ||
| (stopTime - startTime) + | ||
| 'ms' | ||
| ); | ||
| Assert.isTrue( | ||
| (stopTime - startTime) < 10000, | ||
| 'expected time to create and insert a list of 200 accounts to be less than 1500ms' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectsWith200AccountsNoCustomDefaultsPositive() { | ||
| Test.startTest(); | ||
| SObjectFactory.createSObjects(new Account(), 200, null, true); | ||
| Test.stopTest(); | ||
| Account[] checkAccounts = [SELECT Id FROM Account]; | ||
| Assert.areEqual( | ||
| 200, | ||
| checkAccounts.size(), | ||
| 'expected 200 accounts to be created' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectsWith200AccountsWithCustomDefaultsPositive() { | ||
| Test.startTest(); | ||
| SObjectFactory.createSObjects( | ||
| new Account(), | ||
| 200, | ||
| 'SomeDefaultsClass', | ||
| true | ||
| ); | ||
| Test.stopTest(); | ||
| Account[] checkAccounts = [SELECT Id FROM Account]; | ||
| Assert.areEqual( | ||
| 200, | ||
| checkAccounts.size(), | ||
| 'expected 200 accounts to be created' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectsWith200AccountsWithCustomDefaultsProvesUniqueIdsPositive() { | ||
| Test.startTest(); | ||
| List<Account> checkAccounts = (List<Account>) SObjectFactory.createSObjects( | ||
| new Account(), | ||
| 200, | ||
| 'SomeDefaultsClass', | ||
| false | ||
| ); | ||
| Test.stopTest(); | ||
| Set<Id> fakeIds = new Set<Id>(); | ||
| for (Account accountToCheck : checkAccounts) { | ||
| fakeIds.add(accountToCheck.Id); | ||
| } | ||
| Assert.areEqual( | ||
| 200, | ||
| fakeIds.size(), | ||
| 'Expected a set of unique ids to be the same size as the requested number of objects to be created' | ||
| ); | ||
| } | ||
|
|
||
| @IsTest | ||
| static void funcTestCreateSObjectWithAccountAndInsertPositive() { | ||
| Test.startTest(); | ||
| SObjectFactory.createSObject(new Account(), 'SomeDefaultsClass', true); | ||
| Test.stopTest(); | ||
| Account[] checkAccounts = [SELECT Id FROM Account]; | ||
| Assert.areEqual( | ||
| 1, | ||
| checkAccounts.size(), | ||
| 'expected 1 accounts to be created' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests in this class are well written and cover a variety of scenarios. However, there is a potential issue with the way time is being measured for profiling. The Test.startTest() and Test.stopTest() methods are used to reset governor limits in Apex tests, but they do not affect the system clock or the return value of Datetime.now().getTime(). This means that the time measurements include the time taken to start and stop the test context, which may not be what you intended.
To measure the time taken by the code under test more accurately, consider moving the calls to Test.startTest() and Test.stopTest() outside of the time measurement, like so:
- Test.startTest();
- Long startTime = Datetime.now().getTime();
+ Long startTime = Datetime.now().getTime();
+ Test.startTest();
// Code under test...
Test.stopTest();
- Long stopTime = Datetime.now().getTime();
+ Long stopTime = Datetime.now().getTime();This change should be applied to all tests where time measurement is being done. For example, lines 9-14, 28-33, 47-52, 66-71, 83-88, 102-103, and 115-116.
1 participant
Refactors the TestFactory class into a handful of classes that follow the SRP. Renames it to SObjectFactory
Summary by CodeRabbit
TestFactoryinto multiple specialized classes (PermissionsHelper,SObjectFactory,SObjectFactoryDefaults,SObjectFactoryHelper,SObjectFactoryTests,SomeDefaultsClass,UserFactory,UserFactoryHelper). This change improves code maintainability and readability.