Difficult to test example refactoring
Together with my college we have identified some extremely difficult to test code. It was challenging but we succeeded to make it testable and as a side effect it become a better code.
Rules:
- Separate command and query methods. Query is the method which is reading state. Command is altering system state.
- If a method has side effect (see command) then it should be part of an object (never static method)
- Output of static method must be relying on input parameters only (real function)
Function to test:
1/** 2 * called before the associated job has been executed to check whether a lock 3 * file is present that indicates that another extraction request has not completed yet 4 * @param context 5 * @param jobException 6 */ 7public static boolean isExtractionInProgress(Properties props, String pJobName, boolean pCreateLockFile) { 8 boolean debug = log.isDebugEnabled(); 9 boolean isExtractionInProgress = true; // block by default 10 11 try { 12 if (props!=null) { 13 String lock_file_name = props.getProperty(pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY); 14 15 if (lock_file_name == null) { 16 log.error("No job level lock file name found, key used: " + pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY); 17 } else { 18 String sys_base_path = PropertyManager.getInstance(true).getBasePath(); 19 20 if (debug) 21 log.debug("lock_file_name: " + lock_file_name); 22 File lockFile = new File(sys_base_path + lock_file_name); 23 if (lockFile.exists()) { 24 if (debug) 25 log.debug("lock file exists: extraction already in progress"); 26 } else { 27 if (debug) 28 log.debug("lock file does not exist"); 29 30 if (pCreateLockFile) { 31 if (lockFile.createNewFile()) { 32 if (debug) 33 log.debug("Created lock file"); 34 } else { 35 log.error("Can not create job lock file"); 36 } 37 } 38 isExtractionInProgress = false; 39 } 40 } 41 } else { 42 log.error("Can not check lock file: no props found"); 43 } 44 } 45 catch (Exception ex) { 46 log.error("Can not update job completed 'flag' file", ex); 47 } 48 return isExtractionInProgress; 49}
Goal:
- Write unit test which able to test all major cases of this function. It is not a goal to fix potentially incorrect behavior. It is a legacy system and the client code is relying on the behavior.
What are the problems?
-
code layout: as you can see all major activity is behind several
if
close. I personally prefer shortcut evaluation because it makes code more readable. (line 12, 15, 23) - certain edge cases cannot be test because it is logged only. (e.g.: line 42, 46)
- false name: although method name (and javadoc comments too) is describing behavior as a query function it is not really that. At line 31 it is modifying system state by creating file.
- hardcoded non replaceable dependency: at line 18 is using some kind of singleton object to read some data.
As it is visible it is extremely difficult to test as something standalone, without any “safe” refactoring. Safe refactoring means that has IDE support and simple enough to have minimal risk of altering behavior.
Test cases intend to write:
- input validation: although there is no significant input validation and it is not even expected (returning true if any of the input is not valid enough) i would like to handle them explicitly)
- what happening when lock file exists
- what happening when there is no lock file
- testing file system security issues (non readable folder, etc)
As I have mentioned before it is extremely difficult to test so I have done it in multiple iteration to explain each step and provide justification too.
Iteration 1: minimal test of major failer and success cases.
And a little bit refactored method:
1 public static boolean isExtractionInProgress(Properties props, 2 String pJobName, boolean pCreateLockFile) { 3 return isExtractionInProgress(props, pJobName, pCreateLockFile, PropertyManager.getInstance(true)); 4 } 5 6 public static boolean isExtractionInProgress(Properties props, 7 String pJobName, boolean pCreateLockFile, PropertyManager propertyManager) { 8 boolean isExtractionInProgress = true; 9 try { 10 Preconditions.checkNotNull(propertyManager); 11 isExtractionInProgress_paramCheck(props, pJobName); 12 } catch (Exception e) { 13 log.error(e.getMessage(), e); 14 return isExtractionInProgress; 15 } 16 final String key = pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY; 17 final String lock_file_name = props.getProperty(key); 18 final String sys_base_path = propertyManager.getBasePath(); 19 File lockFile = new File(sys_base_path + lock_file_name); 20 try { 21 if (lockFile.exists()) { 22 return isExtractionInProgress; 23 } 24 if (pCreateLockFile) { 25 lockFile.createNewFile()) 26 } 27 isExtractionInProgress = false; 28 } catch (Exception ex) {;} 29 return isExtractionInProgress; 30 } 31 32 static void isExtractionInProgress_paramCheck(Properties props, String pJobName) { 33 if (null == props) { 34 throw new NullPointerException("Can not check lock file: no props found"); 35 } 36 final String key = pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY; 37 if (!props.containsKey(key) || null == props.getProperty(key)) { 38 throw new NullPointerException( 39 "No job level lock file name found, key used: " + 40 pJobName + 41 PericlesJobScheduler.LOCK_FILE_NAME_KEY); 42 } 43 }
Changes:
- the hardcoded dependency is extracted to a an input parameter. the original method is kept and delegating all the parameter to the new method plus the previously hardcoded dependency. Lines 3 and 6.
- Lines 9-15: As in the original implementation input validation cannot be checked clearly (more precisely, not clearly separated from other reasons producing the same output). I have extracted input validation method (for me it is a save refactoring) and finally i have tested the new method (
isExtractionInProgress_paramCheck
) instead. - changed code structure to use shortcuts instead of indented `if closes. Example: line 21
- to make it more readable: eliminated logging. has no functional value but in the original implementation i have kept as before.
Lets see test cases
1public class JobUtils_isExtractionInProgress_Test { 2 Properties prop; 3 PropertyManager propertyManager; 4 File rootDir; 5 File lockFile; 6 String newLockFileName = "newLockfile.txt"; 7 8 @Before 9 public void setup() throws Exception { 10 prop = new Properties(); 11 rootDir = createTempDirectory(); 12 lockFile = new File(rootDir, "testLockFile.txt"); 13 lockFile.createNewFile(); 14 propertyManager = mock(PropertyManager.class); 15 } 16 17 @After 18 public void teardown() throws Exception { 19 lockFile.delete(); 20 new File(rootDir, newLockFileName).delete(); 21 rootDir.delete(); 22 23 } 24 25 public static File createTempDirectory() throws IOException { 26 final File temp; 27 temp = File.createTempFile("temp", Long.toString(System.nanoTime())); 28 if (!(temp.delete())) { 29 throw new IOException("Could not delete temp file: " + temp.getAbsolutePath()); 30 } 31 if (!(temp.mkdir())) { 32 throw new IOException("Could not create temp directory: " + temp.getAbsolutePath()); 33 } 34 return (temp); 35 } 36 37 @Test 38 public void no_property() { 39 new AssertThrows(NullPointerException.class) { 40 @Override 41 public void test() throws Exception { 42 isExtractionInProgress_paramCheck(null, null); 43 } 44 }.runTest(); 45 } 46 47 @Test 48 public void inpropert_job_name() throws Exception { 49 new AssertThrows(NullPointerException.class) { 50 @Override 51 public void test() throws Exception { 52 isExtractionInProgress_paramCheck(prop, null); 53 } 54 }.runTest(); 55 } 56 57 @Test 58 public void lock_exists() throws Exception { 59 prop.setProperty("nullJobLockFileNameKey", lockFile.getName()); 60 when(propertyManager.getBasePath()).thenReturn(rootDir.getAbsolutePath() + File.separator); 61 assertTrue(isExtractionInProgress(prop, null, false, propertyManager)); 62 } 63 64 @Test 65 public void lock_not_exists() throws Exception { 66 prop.setProperty("nullJobLockFileNameKey", newLockFileName); 67 when(propertyManager.getBasePath()).thenReturn(rootDir.getAbsolutePath() + File.separator); 68 assertFalse(isExtractionInProgress(prop, null, false, propertyManager)); 69 } 70}
Changes:
- created: there were no test case for that
- As the method is working on files and folders it make sense to simulate needed environment.
- Temp folder creation a little bit tricky but not so difficult. line 25.
- input validation is made against the new create input validation function. e.g. line 38.
- dependent object is mocked out (by Mockito). Line 60, 67.
In this structure it is not really possible to improve testability. To be able to test file system security option I need full control on File
creation.
Iteration 2: Make it object
1 public static boolean isExtractionInProgress(Properties props, String pJobName, boolean pCreateLockFile, PropertyManager propertyManager) { 2 return new FileLocking(props, pJobName, pCreateLockFile, propertyManager).isExtractionInProgress(); 3 } 4 5 static class FileLocking { 6 private final Properties props; 7 private final String pJobName; 8 private final boolean pCreateLockFile; 9 private final PropertyManager propertyManager; 10 11 public FileLocking(Properties props, String pJobName, boolean pCreateLockFile, PropertyManager propertyManager) { 12 super(); 13 this.props = props; 14 this.pJobName = pJobName; 15 this.pCreateLockFile = pCreateLockFile; 16 this.propertyManager = propertyManager; 17 } 18 19 public boolean isExtractionInProgress() { 20 isExtractionInProgress_paramCheck(); 21 Preconditions.checkNotNull(propertyManager); 22 boolean isExtractionInProgress = true; // block by default 23 final String key = pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY; 24 final String lock_file_name = props.getProperty(key); 25 final String sys_base_path = propertyManager.getBasePath(); 26 final File lockFile = new File(sys_base_path + lock_file_name); 27 try { 28 if (lockFile.exists()) { 29 return isExtractionInProgress; 30 } 31 if (pCreateLockFile) { 32 if (lockFile.createNewFile()) {}else {} 33 } 34 isExtractionInProgress = false; 35 } catch (Exception ex) { 36 } 37 return isExtractionInProgress; 38 } 39 40 void isExtractionInProgress_paramCheck() { 41 if (null == props) { 42 throw new NullPointerException("Can not check lock file: no props found"); 43 } 44 final String key = pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY; 45 if (!props.containsKey(key) || null == props.getProperty(key)) { 46 throw new NullPointerException("No job level lock file name found, key used: " + pJobName 47 + PericlesJobScheduler.LOCK_FILE_NAME_KEY); 48 } 49 } 50 }
As the whole functionality is about locking and implemented through file system I have moved the method into a new class FileLocking
and converted static method into an instance one. Applied the changes on the unit test too, so from no the FileLocking
instance is tested.
I have implemented parameter passing ans constructor call so parameter become field of FileLocking
. Other option would be to have no instance fields and all parameters would be passed to instance method (full delegate). Which option to choose is really up to you and the context.
Iteration 3: testing file system security
1public static boolean isExtractionInProgress(Properties props, String pJobName, boolean pCreateLockFile, PropertyManager propertyManager) { 2 try { 3 return new FileLocking(props, pJobName, pCreateLockFile, propertyManager).checkLockOrCreate(); 4 } catch (Exception e) { 5 log.error(e, e); 6 return true;//??? !!!! ??? 7 } 8 } 9 10 static class FileLocking { 11 private final Properties props; 12 private final String pJobName; 13 private final boolean pCreateLockFile; 14 private final PropertyManager propertyManager; 15 16 public FileLocking(Properties props, String pJobName, boolean pCreateLockFile, PropertyManager propertyManager) { 17 super(); 18 this.props = props; 19 this.pJobName = pJobName; 20 this.pCreateLockFile = pCreateLockFile; 21 this.propertyManager = propertyManager; 22 } 23 24 public boolean checkLockOrCreate() { 25 isExtractionInProgress_paramCheck(); 26 Preconditions.checkNotNull(propertyManager); 27 boolean isExtractionInProgress = true; // block by default 28 final String key = pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY; 29 final String lock_file_name = props.getProperty(key); 30 final String sys_base_path = propertyManager.getBasePath(); 31 final File lockFile = getLockFile(lock_file_name, sys_base_path); 32 try { 33 if (lockFile.exists()) { 34 log.debug("lock file exists: extraction already in progress"); 35 return isExtractionInProgress; 36 } 37 log.debug("lock file does not exist"); 38 if (pCreateLockFile) { 39 if (lockFile.createNewFile()) {}else {} 40 } 41 isExtractionInProgress = false; 42 } catch (Exception ex) {} 43 return isExtractionInProgress; 44 } 45 46 File getLockFile(final String lock_file_name, final String sys_base_path) { 47 return new File(sys_base_path + lock_file_name); 48 } 49 50 void isExtractionInProgress_paramCheck() { 51 if (null == props) { 52 throw new NullPointerException("Can not check lock file: no props found"); 53 } 54 final String key = pJobName + PericlesJobScheduler.LOCK_FILE_NAME_KEY; 55 if (!props.containsKey(key) || null == props.getProperty(key)) { 56 throw new NullPointerException("No job level lock file name found, key used: " + pJobName 57 + PericlesJobScheduler.LOCK_FILE_NAME_KEY); 58 } 59 } 60 61 }
Changes:
- fragment
new File(sys_base_path + lock_file_name)
is extracted into new method calledgetLockFile
. I will use as an “backdoor” in testing. Line 31 and 46.
So the remaining of test cases:
1 @Test 2 public void non_readable_file_or_folder() throws Exception { 3 prop.setProperty("nullJobLockFileNameKey", lockFile.getName()); 4 FileLocking subject = new FileLocking(prop, null, false, propertyManager) { 5 @Override 6 File getLockFile(String lock_file_name, String sys_base_path) { 7 File r = mock(File.class); 8 when(r.exists()).thenThrow(new SecurityException("fake security exception for `File.exists()`")); 9 return r; 10 } 11 }; 12 assertTrue("Are we sure that this is the correct behavior??????", subject.checkLockOrCreate()); 13 } 14 15 @Test 16 public void non_writable_folder() throws Exception { 17 prop.setProperty("nullJobLockFileNameKey", lockFile.getName()); 18 FileLocking subject = new FileLocking(prop, null, true, propertyManager) { 19 @Override 20 File getLockFile(String lock_file_name, String sys_base_path) { 21 File r = mock(File.class); 22 try { 23 when(r.exists()).thenReturn(false); 24 when(r.createNewFile()).thenThrow(new SecurityException("fake security exception for `File.createNewFile()`")); 25 } catch (IOException e) { 26 throw Throwables.propagate(e); 27 } 28 return r; 29 } 30 }; 31 assertTrue("Are we sure that this is the correct behavior??????", subject.checkLockOrCreate()); 32 }
Changes:
- Creating local subclass of FileLocking
and override getLockfile
- the File
instance returning is mock what I have full control on.
- renamed function isExtractionInProgres
to checkLockOrCreate
because more descriptive.
Finally
Of course it is not ready yet. As you see the original implementation is quite inconsistent especially behavior related to error cases. Another next step could be to alter client code to use FileLocking
directly instead of the static delegate method. And finally it should be split into two separate functionality (see command and query methods).
Technology stack
- Physical software project structure
- Service layer
- Data access layer
- Spring MVC
- Web view
- Toolbox
- Testing
- Jawr, webjars, bootstrap, Spring setup trick
- Jakarta Equivalence Relation
- Difficult to test example refactoring