From 9c88c5ccd09b9b9e20d263de7d1e9e03799b646d Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 6 Aug 2024 17:18:23 +0100 Subject: [PATCH] jinja2: removed excessive context from Jinja2 raise/assert statements --- changes.d/6289.feat.md | 1 + cylc/flow/parsec/jinja2support.py | 26 ++++++- tests/functional/jinja2/03-bad.t | 23 +----- .../functional/jinja2/10-builtin-functions.t | 38 ---------- .../jinja2/10-builtin-functions/flow.cylc | 13 ---- .../jinja2/include-exception/flow.cylc | 4 - .../jinja2/include-exception/foo.cylc | 3 - tests/integration/validate/test_jinja2.py | 76 +++++++++++++++++++ tests/unit/parsec/test_jinja2support.py | 9 +-- 9 files changed, 102 insertions(+), 91 deletions(-) create mode 100644 changes.d/6289.feat.md delete mode 100644 tests/functional/jinja2/10-builtin-functions.t delete mode 100644 tests/functional/jinja2/10-builtin-functions/flow.cylc delete mode 100644 tests/functional/jinja2/include-exception/flow.cylc delete mode 100644 tests/functional/jinja2/include-exception/foo.cylc create mode 100644 tests/integration/validate/test_jinja2.py diff --git a/changes.d/6289.feat.md b/changes.d/6289.feat.md new file mode 100644 index 00000000000..9284a768f2f --- /dev/null +++ b/changes.d/6289.feat.md @@ -0,0 +1 @@ +Made the errors resulting from Jinja2 `raise` and `assert` statements more straight forward. diff --git a/cylc/flow/parsec/jinja2support.py b/cylc/flow/parsec/jinja2support.py index 74335358421..be6b99eb19b 100644 --- a/cylc/flow/parsec/jinja2support.py +++ b/cylc/flow/parsec/jinja2support.py @@ -38,6 +38,8 @@ TemplateSyntaxError) from cylc.flow import LOG +from cylc.flow.exceptions import InputError +import cylc.flow.flags from cylc.flow.parsec.exceptions import Jinja2Error from cylc.flow.parsec.fileparse import get_cylc_env_vars @@ -91,16 +93,19 @@ def root_render_func(context, *args, **kwargs): return templ -def raise_helper(message, error_type='Error'): +class Jinja2AssertionError(Exception): + """Exception raised by the Jinja2 "raise" and "assert" functions.""" + + +def raise_helper(message): """Provides a Jinja2 function for raising exceptions.""" - # TODO - this more nicely - raise Exception('Jinja2 %s: %s' % (error_type, message)) + raise Jinja2AssertionError(message) def assert_helper(logical, message): """Provides a Jinja2 function for asserting logical expressions.""" if not logical: - raise_helper(message, 'Assertion Error') + raise_helper(message) return '' # Prevent None return value polluting output. @@ -298,6 +303,19 @@ def jinja2process( lines=get_error_lines(fpath, flines), filename=filename ) from None + except Jinja2AssertionError as exc: + if cylc.flow.flags.verbosity < 1: + # raise a user-friently representation of this assertion error + raise InputError( + f'{str(exc)}' + '\n(add --verbose for more context)' + ) from None + + # raise the full form of the error in verbose mode + raise Jinja2Error( + exc, + lines=get_error_lines(fpath, flines), + ) from None except Exception as exc: raise Jinja2Error( exc, diff --git a/tests/functional/jinja2/03-bad.t b/tests/functional/jinja2/03-bad.t index 5741932e8dc..8d6688183c7 100644 --- a/tests/functional/jinja2/03-bad.t +++ b/tests/functional/jinja2/03-bad.t @@ -20,7 +20,7 @@ # an include statement . "$(dirname "$0")/test_header" -set_test_number 12 +set_test_number 10 sub_tabs() { sed -i 's/\t/ /g' "${TEST_NAME}.stderr" @@ -51,27 +51,6 @@ __HERE__ purge_workflow -#------------------------------------------------------------------------------ -# Test generic exception in an include file -install_workflow "${TEST_NAME_BASE}" include-exception - -TEST_NAME="${TEST_NAME_BASE}-include-exception" -run_fail "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" - -sub_tabs -cmp_ok "${TEST_NAME}.stderr" <<__HERE__ -Jinja2Error: Jinja2 Error: some error -File ${WORKFLOW_RUN_DIR}/foo.cylc - # line before error - {{ raise('some error') }} <-- Exception -File ${WORKFLOW_RUN_DIR}/flow.cylc - #!Jinja2 - # line before - {% include "foo.cylc" %} <-- Exception -__HERE__ - -purge_workflow - #------------------------------------------------------------------------------ # Test syntax error in an include file install_workflow "${TEST_NAME_BASE}" include-badsyntax diff --git a/tests/functional/jinja2/10-builtin-functions.t b/tests/functional/jinja2/10-builtin-functions.t deleted file mode 100644 index c9016f795f6..00000000000 --- a/tests/functional/jinja2/10-builtin-functions.t +++ /dev/null @@ -1,38 +0,0 @@ -#!/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 . -#------------------------------------------------------------------------------- -# jinja2 test cylc-provided functions. -. "$(dirname "$0")/test_header" -#------------------------------------------------------------------------------- -set_test_number 5 -#------------------------------------------------------------------------------- -install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}" -#------------------------------------------------------------------------------- -TEST_NAME="${TEST_NAME_BASE}"-pass -run_ok "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" \ - -s 'FOO="True"' \ - -s 'ANSWER="42"' -TEST_NAME="${TEST_NAME_BASE}"-fail-assert -run_fail "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" \ - -s 'FOO="True"' \ - -s 'ANSWER="43"' -grep_ok 'Jinja2 Assertion Error: Universal' "${TEST_NAME}.stderr" -TEST_NAME="${TEST_NAME_BASE}"-fail-raise -run_fail "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" -s 'ANSWER="42"' -grep_ok 'Jinja2 Error: FOO must be defined' "${TEST_NAME}.stderr" -#------------------------------------------------------------------------------- -purge diff --git a/tests/functional/jinja2/10-builtin-functions/flow.cylc b/tests/functional/jinja2/10-builtin-functions/flow.cylc deleted file mode 100644 index 704704f438c..00000000000 --- a/tests/functional/jinja2/10-builtin-functions/flow.cylc +++ /dev/null @@ -1,13 +0,0 @@ -#!Jinja2 - -{% if not FOO is defined %} - {{ raise('FOO must be defined.') }} -{% endif %} - -{{ assert(ANSWER | int == 42, 'Universal constant incorrectly set.') }} - -[scheduling] - [[graph]] - R1 = foo -[runtime] - [[foo]] diff --git a/tests/functional/jinja2/include-exception/flow.cylc b/tests/functional/jinja2/include-exception/flow.cylc deleted file mode 100644 index 8486a2ebb9c..00000000000 --- a/tests/functional/jinja2/include-exception/flow.cylc +++ /dev/null @@ -1,4 +0,0 @@ -#!Jinja2 -# line before -{% include "foo.cylc" %} -# line after diff --git a/tests/functional/jinja2/include-exception/foo.cylc b/tests/functional/jinja2/include-exception/foo.cylc deleted file mode 100644 index 77d507554b6..00000000000 --- a/tests/functional/jinja2/include-exception/foo.cylc +++ /dev/null @@ -1,3 +0,0 @@ -# line before error -{{ raise('some error') }} -# line after error diff --git a/tests/integration/validate/test_jinja2.py b/tests/integration/validate/test_jinja2.py new file mode 100644 index 00000000000..8b2eae2c797 --- /dev/null +++ b/tests/integration/validate/test_jinja2.py @@ -0,0 +1,76 @@ +# 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 . + +import re +from textwrap import dedent + +from cylc.flow.exceptions import InputError +from cylc.flow.parsec.exceptions import Jinja2Error + +import pytest + + +@pytest.fixture +def flow_cylc(tmp_path): + """Write a flow.cylc file containing the provided text.""" + def _inner(text): + nonlocal tmp_path + (tmp_path / 'flow.cylc').write_text(dedent(text).strip()) + return tmp_path + + return _inner + + +@pytest.mark.parametrize( + 'line', + [ + pytest.param("raise('some error message')", id='raise'), + pytest.param("assert(False, 'some error message')", id='assert'), + ], +) +def test_raise_helper(flow_cylc, validate, line, monkeypatch): + """It should raise an error from within Jinja2.""" + # it should raise a minimal InputError + # (because assert/raise are used to validate inputs) + # - intended for users of the workflow + src_dir = flow_cylc(f''' + #!Jinja2 + {{{{ {line} }}}} + ''') + with pytest.raises( + InputError, + match=( + r'^some error message' + r'\n\(add --verbose for more context\)$' + ), + ): + validate(src_dir) + + # in verbose mode, it should raise the full error + # (this includes the Jinja2 context including file/line info) + # - intended for developers of the workflow + monkeypatch.setattr('cylc.flow.flags.verbosity', 1) + with pytest.raises( + Jinja2Error, + match=( + r'^some error message' + r'\nFile /.*/pytest-.*/test_.*/flow.cylc' + r'\n #!Jinja2' + r'\n \{\{ ' + re.escape(line) + r' \}\}' + r'\t<-- Jinja2AssertionError$' + ) + ): + validate(src_dir) diff --git a/tests/unit/parsec/test_jinja2support.py b/tests/unit/parsec/test_jinja2support.py index 68fb450d4cc..385769463dc 100644 --- a/tests/unit/parsec/test_jinja2support.py +++ b/tests/unit/parsec/test_jinja2support.py @@ -24,15 +24,10 @@ def test_raise_helper(): message = 'Ops' error_type = 'CRITICAL' - with pytest.raises(Exception) as cm: + with pytest.raises(Jinja2AssertionError) as cm: raise_helper(message=message) - assert str(cm.value) == "Jinja2 Error: Ops" - - with pytest.raises(Exception) as cm: - raise_helper(message=message, error_type=error_type) - - assert str(cm.value) == "Jinja2 CRITICAL: Ops" + assert str(cm.value) == "Ops" def test_assert_helper():