Refactor File module to improve test coverage.

Added unit tests for low-level functions in the BackupCommon module.

doc/xml/release.xml

@@ -171,12 +171,18 @@
 
                 <release-refactor-list>
                     <release-item>
-                        <p>Refactor <code>File</code> module to improve test coverage.</p>
+                        <p>Refactor <code>File</code> and <code>BackupCommon</code> modules to improve test coverage.</p>
                     </release-item>
                 </release-refactor-list>
             </release-core-list>
 
             <release-test-list>
+                <release-feature-list>
+                    <release-item>
+                        <p>Added unit tests for low-level functions in the <code>File</code> and <code>BackupCommon</code> modules.</p>
+                    </release-item>
+                </release-feature-list>
+
                 <release-refactor-list>
                     <release-item>
                         <p>Split test modules into separate files to make the code more maintainable.  Tests are dynamically loaded by name rather than requiring an if-else block.</p>

lib/pgBackRest/BackupCommon.pm

@@ -22,7 +22,9 @@ use constant LINK_LATEST                                            => OPTION_DE
     push @EXPORT, qw(LINK_LATEST);
 
 ####################################################################################################################################
-# backupRegExpGet - Generate a regexp depending on the backups that need to be found
+# backupRegExpGet
+#
+# Generate a regexp depending on the backups that need to be found.
 ####################################################################################################################################
 sub backupRegExpGet
 {
@@ -44,51 +46,58 @@ sub backupRegExpGet
             {name => 'bAnchor', default => true}
         );
 
-    if (!$bFull && !$bDifferential && !$bIncremental)
+    # One of the types must be selected
+    if (!($bFull || $bDifferential || $bIncremental))
     {
-        confess &log(ASSERT, 'one parameter must be true');
+        confess &log(ASSERT, 'at least one backup type must be selected');
     }
 
+    # Standard regexp to match date and time formattting
     my $strDateTimeRegExp = "[0-9]{8}\\-[0-9]{6}";
-    my $strRegExp = $bAnchor ? '^' : '';
-
-    if ($bFull || $bDifferential || $bIncremental)
-    {
-        $strRegExp .= $strDateTimeRegExp . 'F';
-    }
+    # Start the expression with the anchor if requested, date/time regexp and full backup indicator
+    my $strRegExp = ($bAnchor ? '^' : '') . $strDateTimeRegExp . 'F';
 
+    # Add the diff and/or incr expressions if requested
     if ($bDifferential || $bIncremental)
     {
+        # If full requested then diff/incr is optional
         if ($bFull)
         {
             $strRegExp .= "(\\_";
         }
+        # Else diff/incr is required
         else
         {
             $strRegExp .= "\\_";
         }
 
+        # Append date/time regexp for diff/incr
         $strRegExp .= $strDateTimeRegExp;
 
+        # Filter on both diff/incr
         if ($bDifferential && $bIncremental)
         {
             $strRegExp .= '(D|I)';
         }
+        # Else just diff
         elsif ($bDifferential)
         {
             $strRegExp .= 'D';
         }
+        # Else just incr
         else
         {
             $strRegExp .= 'I';
         }
 
+        # If full requested then diff/incr is optional
         if ($bFull)
         {
             $strRegExp .= '){0,1}';
         }
     }
 
+    # Append the end anchor if requested
     $strRegExp .= $bAnchor ? "\$" : '';
 
     # Return from function and log return values if any
@@ -103,6 +112,8 @@ push @EXPORT, qw(backupRegExpGet);
 
 ####################################################################################################################################
 # backupLabelFormat
+#
+# Format the label for a backup.
 ####################################################################################################################################
 sub backupLabelFormat
 {
@@ -122,32 +133,41 @@ sub backupLabelFormat
             {name => 'lTimestampStop', trace => true}
         );
 
+    # Full backup label
     my $strBackupLabel;
 
     if ($strType eq BACKUP_TYPE_FULL)
     {
+        # Last backup label must not be defined
         if (defined($strBackupLabelLast))
         {
-            confess &log(ASSERT, "strBackupPathLast cannot be defined when type = ${strType}");
+            confess &log(ASSERT, "strBackupLabelLast must not be defined when strType = '${strType}'");
         }
 
+        # Format the timestamp and add the full indicator
         $strBackupLabel = timestampFileFormat(undef, $lTimestampStop) . 'F';
     }
+    # Else diff or incr label
     else
     {
+        # Last backup label must be defined
         if (!defined($strBackupLabelLast))
         {
-            confess &log(ASSERT, "strBackupLabelLast must be defined when type = ${strType}");
+            confess &log(ASSERT, "strBackupLabelLast must be defined when strType = '${strType}'");
         }
 
+        # Get the full backup portion of the last backup label
         $strBackupLabel = substr($strBackupLabelLast, 0, 16);
 
+        # Format the timestamp
         $strBackupLabel .= '_' . timestampFileFormat(undef, $lTimestampStop);
 
+        # Add the diff indicator
         if ($strType eq BACKUP_TYPE_DIFF)
         {
             $strBackupLabel .= 'D';
         }
+        # Else incr indicator
         else
         {
             $strBackupLabel .= 'I';

test/lib/pgBackRestTest/Backup/BackupUnitTest.pm

@@ -0,0 +1,110 @@
+####################################################################################################################################
+# BackupUnitTest.pm - Tests for Backup module
+####################################################################################################################################
+package pgBackRestTest::Backup::BackupUnitTest;
+use parent 'pgBackRestTest::Backup::BackupCommonTest';
+
+####################################################################################################################################
+# Perl includes
+####################################################################################################################################
+use strict;
+use warnings FATAL => qw(all);
+use Carp qw(confess);
+
+use File::Basename qw(dirname);
+
+use pgBackRest::BackupCommon;
+use pgBackRest::Common::Exception;
+use pgBackRest::Common::Log;
+use pgBackRest::Common::String;
+use pgBackRest::Config::Config;
+
+####################################################################################################################################
+# run
+####################################################################################################################################
+sub run
+{
+    my $self = shift;
+
+    # Increment the run, log, and decide whether this unit test should be run
+    if (!$self->begin('unit')) {return}
+
+    # Unit tests for backupLabelFormat()
+    #-----------------------------------------------------------------------------------------------------------------------
+    {
+        # Test full backup label
+        my $strBackupLabelFull = timestampFileFormat(undef, 1482000000) . 'F';
+        $self->testResult(sub {backupLabelFormat(BACKUP_TYPE_FULL, undef, 1482000000)}, $strBackupLabelFull);
+
+        # Make sure that an assertion is thrown if strBackupLabelLast is passed when formatting a full label
+        $self->testException(
+            sub {backupLabelFormat(BACKUP_TYPE_FULL, $strBackupLabelFull, 1482000000)},
+            ERROR_ASSERT, "strBackupLabelLast must not be defined when strType = 'full'");
+
+        # Test diff backup label
+        my $strBackupLabelDiff = "${strBackupLabelFull}_" . timestampFileFormat(undef, 1482000400) . 'D';
+        $self->testResult(sub {backupLabelFormat(BACKUP_TYPE_DIFF, $strBackupLabelFull, 1482000400)}, $strBackupLabelDiff);
+
+        # Make sure that an assertion is thrown if strBackupLabelLast is not passed when formatting a diff label.  The same
+        # check is used from incr labels so no need for a separate test.
+        $self->testException(
+            sub {backupLabelFormat(BACKUP_TYPE_DIFF, undef, 1482000400)},
+            ERROR_ASSERT, "strBackupLabelLast must be defined when strType = 'diff'");
+
+        # Test incr backup label
+        $self->testResult(
+            sub {backupLabelFormat(BACKUP_TYPE_INCR, $strBackupLabelDiff, 1482000800)},
+            "${strBackupLabelFull}_" . timestampFileFormat(undef, 1482000800) . 'I');
+    }
+
+    # Unit tests for backupRegExpGet()
+    #-----------------------------------------------------------------------------------------------------------------------
+    {
+        # Expected results matrix
+        my $hExpected = {};
+        $hExpected->{&false}{&false}{&true}{&false} = '[0-9]{8}\-[0-9]{6}F\_[0-9]{8}\-[0-9]{6}I';
+        $hExpected->{&false}{&false}{&true}{&true} = '^[0-9]{8}\-[0-9]{6}F\_[0-9]{8}\-[0-9]{6}I$';
+        $hExpected->{&false}{&true}{&false}{&false} = '[0-9]{8}\-[0-9]{6}F\_[0-9]{8}\-[0-9]{6}D';
+        $hExpected->{&false}{&true}{&false}{&true} = '^[0-9]{8}\-[0-9]{6}F\_[0-9]{8}\-[0-9]{6}D$';
+        $hExpected->{&false}{&true}{&true}{&false} = '[0-9]{8}\-[0-9]{6}F\_[0-9]{8}\-[0-9]{6}(D|I)';
+        $hExpected->{&false}{&true}{&true}{&true} = '^[0-9]{8}\-[0-9]{6}F\_[0-9]{8}\-[0-9]{6}(D|I)$';
+        $hExpected->{&true}{&false}{&false}{&false} = '[0-9]{8}\-[0-9]{6}F';
+        $hExpected->{&true}{&false}{&false}{&true} = '^[0-9]{8}\-[0-9]{6}F$';
+        $hExpected->{&true}{&false}{&true}{&false} = '[0-9]{8}\-[0-9]{6}F(\_[0-9]{8}\-[0-9]{6}I){0,1}';
+        $hExpected->{&true}{&false}{&true}{&true} = '^[0-9]{8}\-[0-9]{6}F(\_[0-9]{8}\-[0-9]{6}I){0,1}$';
+        $hExpected->{&true}{&true}{&false}{&false} = '[0-9]{8}\-[0-9]{6}F(\_[0-9]{8}\-[0-9]{6}D){0,1}';
+        $hExpected->{&true}{&true}{&false}{&true} = '^[0-9]{8}\-[0-9]{6}F(\_[0-9]{8}\-[0-9]{6}D){0,1}$';
+        $hExpected->{&true}{&true}{&true}{&false} = '[0-9]{8}\-[0-9]{6}F(\_[0-9]{8}\-[0-9]{6}(D|I)){0,1}';
+        $hExpected->{&true}{&true}{&true}{&true} = '^[0-9]{8}\-[0-9]{6}F(\_[0-9]{8}\-[0-9]{6}(D|I)){0,1}$';
+
+        # Iterate though all possible combinations
+        for (my $bFull = false; $bFull <= true; $bFull++)
+        {
+        for (my $bDiff = false; $bDiff <= true; $bDiff++)
+        {
+        for (my $bIncr = false; $bIncr <= true; $bIncr++)
+        {
+        for (my $bAnchor = false; $bAnchor <= true; $bAnchor++)
+        {
+            # Make sure that an assertion is thrown if no types are requested
+            if (!($bFull || $bDiff || $bIncr))
+            {
+                $self->testException(
+                    sub {backupRegExpGet($bFull, $bDiff, $bIncr, $bAnchor)},
+                    ERROR_ASSERT, 'at least one backup type must be selected');
+            }
+            # Else make sure the returned value is correct
+            else
+            {
+                $self->testResult(
+                    sub {backupRegExpGet($bFull, $bDiff, $bIncr, $bAnchor)},
+                    $hExpected->{$bFull}{$bDiff}{$bIncr}{$bAnchor});
+            }
+        }
+        }
+        }
+        }
+    }
+}
+
+1;

test/lib/pgBackRestTest/Common/DefineTest.pm

@@ -153,6 +153,12 @@ my $oTestDef =
 
             &TESTDEF_TEST =>
             [
+                {
+                    &TESTDEF_TEST_NAME => 'unit',
+                    &TESTDEF_TEST_TOTAL => 1,
+                    &TESTDEF_TEST_INDIVIDUAL => false,
+                    &TESTDEF_EXPECT => false,
+                },
                 {
                     &TESTDEF_TEST_NAME => 'archive-push',
                     &TESTDEF_TEST_TOTAL => 8

test/lib/pgBackRestTest/Common/RunTest.pm

@@ -180,7 +180,13 @@ sub begin
     else
     {
         my $hModule = testDefModuleGet($self->{strModule});
-        $self->{bExpect} = $hModule->{&TESTDEF_EXPECT} ? true : false;
+        my $hModuleTest = testDefModuleTestGet($hModule, $self->{strModuleTest});
+        $self->{bExpect} =
+            defined($hModuleTest->{&TESTDEF_EXPECT}) ?
+                ($hModuleTest->{&TESTDEF_EXPECT} ? true : false) :
+                (defined($hModule->{&TESTDEF_EXPECT}) ?
+                    ($hModule->{&TESTDEF_EXPECT} ? true : false) :
+                    false);
     }
 
     # Increment the run counter;