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:

What are the problems?

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:

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:

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:

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:

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
Sep 01, 2013
comments powered by Disqus

Links

Cool

RSS