Add tests to cover parser errors

The added tests cover the following cases:
- without release name or with invalid characters (except)
- with duplicated release name (warning)
- without opening '{' (except)
- with invalid characters in visibility or symbol name (except)
- missing ';' or ':' after symbol or visibility (except)
- missing visibility scope (warning)
- previous with invalid characters (except)
- previous missing closer ';' (except)

Also removed an impossible state in the parser FSM

src/symbol_version/symbol_version.py

@@ -24,7 +24,7 @@ class Single_Logger(object):
     __instance = None
 
     @classmethod
-    def getLogger(cls, name):
+    def getLogger(cls, name, filename=None):
         """
         Get the unique instance of the logger
 
@@ -35,6 +35,10 @@ def getLogger(cls, name):
             # Get logger
             logger = logging.getLogger(name)
 
+            if filename:
+                file_handler = logging.FileHandler(filename)
+                logger.addHandler(file_handler)
+
             # Setup a handler to print warnings and above to stderr
             console_handler = logging.StreamHandler()
             console_handler.setLevel(logging.WARNING)
@@ -201,12 +205,12 @@ class ParserError(Exception):
 
     def __str__(self):
         content = "".join(["In file ", self.filename,
-                           ", line ", self.line + 1,
-                           ", column ", self.column,
+                           ", line ", str(self.line + 1),
+                           ", column ", str(self.column),
                            ": ", self.message,
                            "\n",
                            self.context,
-                           " " * (self.column - 1),
+                           (" " * (self.column - 1)),
                            "^"])
         return content
 
@@ -326,9 +330,10 @@ def parse(self, lines):
                         self.logger.debug(">>Name")
                         m = re.match(r'\w+', line[column:])
                         if m is None:
-                            raise ParserError(lines[last[0]], last[0],
-                                              last[1], "Invalid Release"
-                                              "identifier")
+                            raise ParserError(self.filename,
+                                              lines[last[0]], last[0],
+                                              last[1],
+                                              "Invalid Release identifier")
                         else:
                             # New release found
                             name = m.group(0)
@@ -343,9 +348,10 @@ def parse(self, lines):
                             state += 1
                             if has_duplicate:
                                 msg = "".join(["Duplicated Release identifier"
-                                               "\'", name, "\'"])
+                                               " \'", name, "\'"])
                                 # This is non-critical, only warning
-                                self.logger.warn(ParserError(lines[index],
+                                self.logger.warn(ParserError(self.filename,
+                                                             lines[index],
                                                              index,
                                                              column, msg))
                             continue
@@ -354,7 +360,8 @@ def parse(self, lines):
                         self.logger.debug(">>Opening")
                         m = re.match(r'\{', line[column:])
                         if m is None:
-                            raise ParserError(lines[last[0]], last[0], last[1],
+                            raise ParserError(self.filename,
+                                              lines[last[0]], last[0], last[1],
                                               "Missing \'{\'")
                         else:
                             column += m.end()
@@ -366,14 +373,15 @@ def parse(self, lines):
                         self.logger.debug(">>Element")
                         m = re.match(r'\}', line[column:])
                         if m:
-                            self.logger.debug(">>Release closer, jump to Previous")
+                            self.logger.debug(">>Closer, jump to Previous")
                             column += m.end()
                             last = (index, column)
                             state = 4
                             continue
                         m = re.match(r'\w+|\*', line[column:])
                         if m is None:
-                            raise ParserError(lines[last[0]], last[0], last[1],
+                            raise ParserError(self.filename,
+                                              lines[last[0]], last[0], last[1],
                                               "Invalid identifier")
                         else:
                             # In this case the position before the
@@ -393,7 +401,8 @@ def parse(self, lines):
                                 msg = "".join(["Missing \';\' or \':\' after",
                                                "\'", identifier, "\'"])
                                 # In this case the current position is used
-                                raise ParserError(lines[index], index, column,
+                                raise ParserError(self.filename,
+                                                  lines[index], index, column,
                                                   msg)
                             else:
                                 # New visibility found
@@ -413,7 +422,8 @@ def parse(self, lines):
                                                " Symbols considered in",
                                                "\'global:\'"])
                                 # Non-critical, only warning
-                                self.logger.warn(ParserError(lines[last[0]],
+                                self.logger.warn(ParserError(self.filename,
+                                                             lines[last[0]],
                                                              last[0], last[1],
                                                              msg))
                             else:
@@ -436,8 +446,9 @@ def parse(self, lines):
                             continue
                         m = re.match(r'\w+', line[column:])
                         if m is None:
-                            raise ParserError(lines[last[0]], last[0], last[1], "Invalid"
-                                              " identifier")
+                            raise ParserError(self.filename,
+                                              lines[last[0]], last[0], last[1],
+                                              "Invalid identifier")
                         else:
                             # Found previous release identifier
                             column += m.end()
@@ -449,7 +460,8 @@ def parse(self, lines):
                         self.logger.debug(">>Previous closer")
                         m = re.match(r'^;', line[column:])
                         if m is None:
-                            raise ParserError(lines[last[0]], last[0], last[1],
+                            raise ParserError(self.filename,
+                                              lines[last[0]], last[0], last[1],
                                               "Missing \';\'")
                         else:
                             # Found previous closer
@@ -459,10 +471,7 @@ def parse(self, lines):
                             # Move back the state to find other releases
                             state = 0
                             continue
-                    else:
-                        # Should never reach this
-                        raise ParserError(lines[last[0]], last[0], last[1], "Unknown"
-                                          "parser state")
+
                 except ParserError as e:
                     # Any exception raised is considered an error
                     self.logger.error(e)
@@ -1012,7 +1021,7 @@ def update(args):
     """
 
     # Get logger
-    logger = Single_Logger.getLogger(__name__)
+    logger = Single_Logger.getLogger(__name__, filename=args.logfile)
 
     logger.info("Command: update")
     logger.debug("Arguments provided: ")
@@ -1038,7 +1047,7 @@ def update(args):
         check_files('--out', args.out, 'file', args.file, args.dry)
 
     # Read the current map file
-    cur_map = Map(filename=args.file)
+    cur_map = Map(filename=args.file, logger=logger)
 
     # Get all global symbols
     all_symbols = list(cur_map.all_global_symbols())
@@ -1220,7 +1229,7 @@ def new(args):
     """
 
     # Get logger
-    logger = Single_Logger.getLogger(__name__)
+    logger = Single_Logger.getLogger(__name__, filename=args.logfile)
 
     logger.info("Command: new")
     logger.debug("Arguments provided: ")
@@ -1344,11 +1353,13 @@ def get_arg_parser():
     file_args.add_argument('-o', '--out',
                            help='Output file (defaults to stdout)')
     file_args.add_argument('-i', '--in',
-                           help='Read from a file instead of stdio',
+                           help='Read from this file instead of stdio',
                            dest='input')
     file_args.add_argument('-d', '--dry',
                            help='Do everything, but do not modify the files',
                            action='store_true')
+    file_args.add_argument('-l', '--logfile',
+                           help='Log to this file')
 
     # Common verbosity arguments
     verb_args = argparse.ArgumentParser(add_help=False)

tests/conftest.py

@@ -2,19 +2,20 @@
 import os
 from distutils import dir_util
 
+import pytest
 import yaml
-from pytest import fixture
 
 from symbol_version import symbol_version
 
 
-@fixture
+@pytest.fixture
 def datadir(tmpdir, request):
-    '''
+    """
     Fixture responsible for searching a folder with the same name of test
     module in the \'data\' directory and, if available, moving all contents
     to a temporary directory so tests can use them freely.
-    '''
+    """
+
     filename = request.module.__file__
     test_dir, _ = os.path.splitext(filename)
 
@@ -32,11 +33,12 @@ def datadir(tmpdir, request):
     return tmpdir
 
 
-@fixture
+@pytest.fixture
 def testcases(datadir):
     """
     Returns the test cases for a given test
     """
+
     input_list = datadir.listdir()
 
     all_tests = []
@@ -57,6 +59,7 @@ def testcases(datadir):
 
 class cd:
     """Class used to manage the working directory"""
+
     def __init__(self, new_path):
         self.new_path = str(new_path)
 
@@ -94,7 +97,13 @@ def run_tc(tc, datadir, capsys, caplog):
                 args.input = tc_in["stdin"]
 
         # Call the function
-        args.func(args)
+        if tc_out["exceptions"]:
+            with pytest.raises(Exception) as e:
+                args.func(args)
+                for expected in tc_out["exceptions"]:
+                    assert expected in str(e.value)
+        else:
+            args.func(args)
 
         # If there is an expected output file
         if tc_out["file"]:
@@ -120,4 +129,7 @@ def run_tc(tc, datadir, capsys, caplog):
         # Check if the expected messages are in the log
         if tc_out["warnings"]:
             for expected in tc_out["warnings"]:
-                assert "WARNING  " + expected in caplog.text
+                assert expected in caplog.text
+
+        # Clear the captured log and output so far
+        caplog.clear()

tests/data/test_update/duplicated.map

@@ -0,0 +1,18 @@
+# Broken map with duplicated releases
+# This is non-critical, only warning generated
+
+LIBTC5_1_0_0
+{
+    global:
+        other_symbol;
+    local:
+        *;
+} ;
+
+LIBTC5_1_0_0
+{
+    global:
+        some_symbol;
+    local:
+        *;
+} ;

tests/data/test_update/invalid_element.map

@@ -0,0 +1,6 @@
+# Broken map with invalid element name
+
+LIBTC5_1_0_0
+{
+    $&*#:
+} ;

tests/data/test_update/invalid_previous.map

@@ -0,0 +1,9 @@
+# Broken map with invalid characters in previous
+
+LIBTC5_6_0_0
+{
+        other_symbol;
+    local:
+        *;
+} $#&@;
+

tests/data/test_update/missing_previous_closer.map

@@ -0,0 +1,9 @@
+# Broken map missing the previous closer ';'
+
+LIBTC5_7_0_0
+{
+        other_symbol;
+    local:
+        *;
+}
+

tests/data/test_update/missing_semicolon.map

@@ -0,0 +1,10 @@
+# Broken map missing a colon or semicolon
+
+LIBTC5_4_0_0
+{
+    global
+        other_symbol;
+    local:
+        *;
+} ;
+

tests/data/test_update/missing_visibility.map

@@ -0,0 +1,10 @@
+# Broken map missing visibility scope
+# Non-critical, only warning generated
+
+LIBTC5_5_0_0
+{
+        other_symbol;
+    local:
+        *;
+} ;
+

tests/data/test_update/nameless.map

@@ -0,0 +1,9 @@
+# Broken map with a release with invalid characters in the name
+
+&^$@
+{
+    global:
+        other_symbol;
+    local:
+        *;
+} ;

tests/data/test_update/opening.map

@@ -0,0 +1,8 @@
+# Broken map missing opening '{'
+
+LIBTC5_1_0_0
+    global:
+        other_symbol;
+    local:
+        *;
+} ;

tests/data/test_update/symbol.in

@@ -0,0 +1 @@
+symbol

tests/data/test_update/tc1.yaml

@@ -1,4 +1,4 @@
-# Testcases to test new command
+# Testing add strategy
 -
   input:
     args:

tests/data/test_update/tc2.yaml

@@ -1,4 +1,4 @@
-# Testcases to test new command
+# Testing remove strategy
 -
   input:
     args:

tests/data/test_update/tc3.yaml

@@ -1,4 +1,4 @@
-# Testcases to test new command
+# Testing symbols strategy
 -
   input:
     args:

tests/data/test_update/tc4.yaml

@@ -1,4 +1,4 @@
-# Testcases to test new command
+# Testing overwriting the map
 -
   input:
     args:

tests/data/test_update/tc5.1.out

@@ -0,0 +1,10 @@
+# This map file was automatically updated
+
+LIBTC5_2_0_0
+{
+    global:
+        symbol;
+    local:
+        *;
+} ;
+

tests/data/test_update/tc5.1.stdout

@@ -0,0 +1,8 @@
+Added:
+    symbol
+
+Removed:
+    other_symbol
+    some_symbol
+
+Merging all symbols in a single new release