jinja2: improve error context information

* When `{% include %}` statements are involved, the error context we
  extract highlights the include statement as the error *not* the actual
  line where the error happened.
* This expands the context extraction to pull out the relevant lines
  from include files where this happens.

cylc/flow/parsec/empysupport.py

@@ -25,7 +25,7 @@
 from cylc.flow.parsec.exceptions import EmPyError
 
 
-def empyprocess(flines, dir_, template_vars=None):
+def empyprocess(_fpath, flines, dir_, template_vars=None):
     """Pass configure file through EmPy processor."""
 
     cwd = os.getcwd()
@@ -38,7 +38,10 @@ def empyprocess(flines, dir_, template_vars=None):
         interpreter.file(ftempl, '<template>', template_vars)
     except Exception as exc:
         lineno = interpreter.contexts[-1].identify()[1]
-        raise EmPyError(str(exc), lines=flines[max(lineno - 4, 0): lineno])
+        raise EmPyError(
+            str(exc),
+            lines={'<template>': flines[max(lineno - 4, 0): lineno]},
+        )
     finally:
         interpreter.shutdown()
         xworkflow = xtempl.getvalue()

cylc/flow/parsec/exceptions.py

@@ -68,7 +68,28 @@ def __str__(self):
 
 
 class FileParseError(ParsecError):
-    """Error raised when attempting to read in the config file(s)."""
+    """Error raised when attempting to read in the config file(s).
+
+    Args:
+        reason:
+            Description of error.
+        err_type:
+            Classification of error (e.g. Jinja2Error).
+        help_lines:
+            Additional info to include in the exception.
+
+    Args (ways of providing exception context):
+        lines:
+            (preferred) Dictionary in the format
+            {filename: [context_line, ..., error_line]}
+        line_num:
+            The line number of the error in the config.
+        line:
+            The line of the error in the config.
+        fpath:
+            The path to the file containing the error.
+
+    """
 
     def __init__(self, reason, index=None, line=None, lines=None,
                  err_type=None, fpath=None, help_lines=None):
@@ -94,10 +115,11 @@ def __str__(self):
         if self.line:
             msg += ":\n   " + self.line.strip()
         if self.lines:
-            msg += "\nContext lines:\n" + "\n".join(self.lines)
-            msg += "\t<--"
-            if self.err_type:
-                msg += ' %s' % self.err_type
+            for filename, lines in self.lines.items():
+                msg += f'\nFile {filename}\n  ' + '\n  '.join(lines)
+                msg += "\t<--"
+                if self.err_type:
+                    msg += ' %s' % self.err_type
         help_lines = list(self.help_lines)
         if self.line_num:
             # TODO - make 'view' function independent of cylc:
@@ -116,7 +138,18 @@ class EmPyError(FileParseError):
 
 
 class Jinja2Error(FileParseError):
-    """Wrapper class for Jinja2 exceptions."""
+    """Wrapper class for Jinja2 exceptions.
+
+    Args:
+        exception:
+            The exception being re-raised
+        lines:
+            Dictionary in the format
+            {filename: [context_line, ..., error_line]}
+        filename:
+            Alternative to "lines" where less detail is available.
+
+    """
 
     def __init__(self, exception, lines=None, filename=None):
         # extract the first sentence of exception

cylc/flow/parsec/fileparse.py

@@ -424,7 +424,7 @@ def read_and_proc(fpath, template_vars=None, viewcfg=None, opts=None):
                 raise ParsecError('EmPy Python package must be installed '
                                   'to process file: ' + fpath)
             flines = empyprocess(
-                flines, fdir, template_vars
+                fpath, flines, fdir, template_vars
             )
 
     # process with Jinja2
@@ -444,7 +444,7 @@ def read_and_proc(fpath, template_vars=None, viewcfg=None, opts=None):
                 raise ParsecError('Jinja2 Python package must be installed '
                                   'to process file: ' + fpath)
             flines = jinja2process(
-                flines, fdir, template_vars
+                fpath, flines, fdir, template_vars
             )
 
     # concatenate continuation lines

cylc/flow/parsec/jinja2support.py

@@ -39,7 +39,9 @@
 from cylc.flow import LOG
 from cylc.flow.parsec.exceptions import Jinja2Error
 
-TRACEBACK_LINENO = re.compile(r'(\s+)?File "<template>", line (\d+)')
+TRACEBACK_LINENO = re.compile(
+    r'\s+?File "(?P<file>.*)", line (?P<line>\d+), in .*template'
+)
 CONTEXT_LINES = 3
 
 
@@ -189,21 +191,44 @@ def jinja2environment(dir_=None):
     return env
 
 
-def get_error_location():
-    """Extract template line number from end of traceback.
+def get_error_lines(base_template_file, template_lines):
+    """Extract exception lines from Jinja2 tracebacks.
 
     Returns:
-        int: The line number or None if not found.
+        {filename: [exception_line, ...]}
+
+        There may be multiple entries due to {% include %} statements.
 
     """
+    ret = {}
     for line in reversed(traceback.format_exc().splitlines()):
         match = TRACEBACK_LINENO.match(line)
+        lines = None
         if match:
-            return int(match.groups()[1])
-    return None
-
-
-def jinja2process(flines, dir_, template_vars=None):
+            filename = match.groupdict()['file']
+            lineno = int(match.groupdict()['line'])
+            start_line = max(lineno - CONTEXT_LINES, 0)
+            if filename == '<template>':
+                filename = base_template_file
+                lineno += 1  # shebang line ignored by jinja2
+                lines = template_lines[start_line:lineno]
+            else:
+                with suppress(FileNotFoundError):  # noqa: SIM117
+                    with open(filename, 'r') as jinja2_file:
+                        jinja2_file.seek(start_line, 0)
+                        lines = [
+                            # use splitlines to remove the newline char at the
+                            # end of the line
+                            jinja2_file.readline().splitlines()[0]
+                            for _ in range(CONTEXT_LINES)
+                        ]
+            if lines:
+                ret[filename] = lines
+
+    return ret
+
+
+def jinja2process(fpath, flines, dir_, template_vars=None):
     """Pass configure file through Jinja2 processor."""
     # Load file lines into a template, excluding '#!jinja2' so that
     # '#!cylc-x.y.z' rises to the top. Callers should handle jinja2
@@ -240,17 +265,10 @@ def jinja2process(flines, dir_, template_vars=None):
                 lines = []
                 for _ in range(CONTEXT_LINES):
                     lines.append(include_file.readline().splitlines()[0])
-        if lines:
-            # extract context lines from source lines
-            lines = lines[max(exc.lineno - CONTEXT_LINES, 0):exc.lineno]
-
+        lines = get_error_lines(fpath, flines)
         raise Jinja2Error(exc, lines=lines, filename=filename)
     except Exception as exc:
-        lineno = get_error_location()
-        lines = None
-        if lineno:
-            lineno += 1  # shebang line ignored by jinja2
-            lines = flines[max(lineno - CONTEXT_LINES, 0):lineno]
+        lines = get_error_lines(fpath, flines)
         raise Jinja2Error(exc, lines=lines)
 
     flow_config = []

tests/functional/jinja2/12-error.t

@@ -0,0 +1,40 @@
+#!/usr/bin/env bash
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#-------------------------------------------------------------------------------
+# ensure parsec can pull out the Jinja2 error lines even when they are behind
+# an include statement
+. "$(dirname "$0")/test_header"
+set_test_number 2
+
+install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}"
+TEST_NAME="${TEST_NAME_BASE}-validate"
+run_fail "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}"
+cmp_ok_re "${TEST_NAME}.stderr" <<__HERE__
+Jinja2Error: .* some error
+File .*foo.cylc
+  # line before error
+  {{ raise\('some error'\) }}
+  # line after error.*<-- Exception
+File .*flow.cylc
+  #!Jinja2
+  
+  # line before include
+  {% include "foo.cylc" %}.*<-- Exception
+__HERE__
+
+purge
+exit

tests/functional/jinja2/12-error/flow.cylc

@@ -0,0 +1,5 @@
+#!Jinja2
+
+# line before include
+{% include "foo.cylc" %}
+# line after include

tests/functional/jinja2/12-error/foo.cylc

@@ -0,0 +1,3 @@
+# line before error
+{{ raise('some error') }}
+# line after error

tests/functional/lib/bash/test_header

@@ -50,6 +50,10 @@
 #         compare files. However, if an alternate command such as "xxdiff -D"
 #         is desirable (e.g. for debugging),
 #         "export CYLC_TEST_DIFF_CMD=xxdiff -D".
+#     cmp_ok_re FILE_TEST FILE_CONTROL
+#         Match a file against a regex file line by line.
+#         Reads in the test and control files line by line and checks the test
+#         line re.match'es the control line.
 #     cmp_json FILE_TEST [FILE_CONTROL]
 #         Alternative implementation to cmp_json_ok, compare two JSON files
 #         and display any differences as a unified(ish) diff:
@@ -311,6 +315,50 @@ cmp_ok() {
     fail "${TEST_NAME}"
 }
 
+cmp_ok_re() {
+    local FILE_TEST="$1"
+    local FILE_CONTROL="${2:--}"
+    local TEST_NAME
+    TEST_NAME="$(basename "${FILE_TEST}")-cmp-ok"
+    if python3 -c '
+import re
+import sys
+
+def compare(test_file, control_file):
+    for ind, (test_line, control_line) in enumerate(
+        zip(test_file, control_file)
+    ):
+        if test_line is None:
+            sys.stderr.write("Test file is missing lines")
+            sys.exit(1)
+        if control_line is None:
+            sys.stderr.write("Test file has extra lines")
+            sys.exit(1)
+        if not re.match(control_line, test_line):
+            sys.stderr.write(
+                f"Line {ind+1}: \"{test_line}\" does not match /{control_line}/"
+                .replace("\n", "")
+            )
+            sys.exit(1)
+
+test_filename, control_filename = sys.argv[1:3]
+with open(test_filename, "r") as test_file:
+    if control_filename == "-":
+        compare(test_file, sys.stdin)
+    else:
+        with open(control_filename, "r") as control_file:
+            compare(test_file, control_file)
+' "${FILE_TEST}" "${FILE_CONTROL}"; then
+        ok "${TEST_NAME}"
+    else
+        (
+            echo -e '\n# Test file:'
+            cat "${FILE_TEST}"
+        ) >&2
+        fail "${TEST_NAME}"
+    fi
+}
+
 cmp_json() {
     local TEST_NAME="$1"
     local FILE_TEST="$2"

tests/functional/validate/40-jinja2-template-syntax-error-main.t

@@ -23,14 +23,14 @@ install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}"
 #-------------------------------------------------------------------------------
 TEST_NAME="${TEST_NAME_BASE}-val"
 run_fail "${TEST_NAME}" cylc validate .
-cmp_ok "${TEST_NAME}.stderr" <<'__ERROR__'
+cmp_ok_re "${TEST_NAME}.stderr" <<'__ERROR__'
 Jinja2Error: Encountered unknown tag 'end'.
 Jinja was looking for the following tags: 'elif' or 'else' or 'endif'.
 The innermost block that needs to be closed is 'if'.
-Context lines:
-        {% if true %}
-        R1 = foo
-        {% end if %	<-- TemplateSyntaxError
+File.*
+          {% if true %}
+          R1 = foo
+          {% end if %	<-- TemplateSyntaxError
 __ERROR__
 #-------------------------------------------------------------------------------
 purge

tests/functional/validate/41-jinja2-template-syntax-error-cylc-include.t

@@ -23,14 +23,14 @@ install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}"
 #-------------------------------------------------------------------------------
 TEST_NAME="${TEST_NAME_BASE}-val"
 run_fail "${TEST_NAME}" cylc validate .
-cmp_ok "${TEST_NAME}.stderr" <<'__ERROR__'
+cmp_ok_re "${TEST_NAME}.stderr" <<'__ERROR__'
 Jinja2Error: Encountered unknown tag 'end'.
 Jinja was looking for the following tags: 'elif' or 'else' or 'endif'.
 The innermost block that needs to be closed is 'if'.
-Context lines:
-        {% if true %}
-        R1 = foo
-        {% end if %	<-- TemplateSyntaxError
+File.*
+          {% if true %}
+          R1 = foo
+          {% end if %	<-- TemplateSyntaxError
 __ERROR__
 #-------------------------------------------------------------------------------
 purge

tests/functional/validate/42-jinja2-template-syntax-error-jinja-include.t

@@ -23,15 +23,15 @@ install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}"
 #-------------------------------------------------------------------------------
 TEST_NAME="${TEST_NAME_BASE}-val"
 run_fail "${TEST_NAME}" cylc validate .
-cmp_ok "${TEST_NAME}.stderr" <<'__ERROR__'
+cmp_ok_re "${TEST_NAME}.stderr" <<'__ERROR__'
 Jinja2Error: Encountered unknown tag 'end'.
 Error in file "flow-includeme.cylc"
 Jinja was looking for the following tags: 'elif' or 'else' or 'endif'.
 The innermost block that needs to be closed is 'if'.
-Context lines:
-        {% if true %}
-        R1 = foo
-        {% end if %	<-- TemplateSyntaxError
+File .*
+          {% if true %}
+          R1 = foo
+          {% end if %	<-- TemplateSyntaxError
 __ERROR__
 #-------------------------------------------------------------------------------
 purge

tests/functional/validate/43-jinja2-template-error-main.t

@@ -31,12 +31,13 @@ cat >'flow.cylc' <<'__FLOW_CONFIG__'
         script = sleep 1
 __FLOW_CONFIG__
 run_fail "${TEST_NAME_BASE}" cylc validate .
-cmp_ok "${TEST_NAME_BASE}.stderr" <<'__ERROR__'
+cmp_ok_re "${TEST_NAME_BASE}.stderr" <<'__ERROR__'
 Jinja2Error: You can only sort by either "key" or "value"
-Context lines:
-[scheduling]
-    [[graph]]
-        R1 = {{ foo|dictsort(by='by') }}	<-- FilterArgumentError
+File.*
+  {% set foo = {} %}
+  \[scheduling\]
+      \[\[graph\]\]
+          R1 = {{ foo\|dictsort\(by='by'\) }}	<-- FilterArgumentError
 __ERROR__
 
 exit