Merge pull request #13152 from bernt-matthias/topic/xml-replace-fix-test

XML macros: add named yields, tokenized macros, tokens for attributes, and fix replacement of toplevel yield

lib/galaxy/util/xml_macros.py

@@ -1,5 +1,5 @@
 import os
-from copy import deepcopy
+from copy import copy, deepcopy
 
 from galaxy.util import parse_xml
 
@@ -17,13 +17,30 @@ def load_with_references(path):
 
     macro_paths = _import_macros(root, path)
 
+    # temporarily remove the children of the macros node
+    # and create a copy. this is done because this allows
+    # to iterate over all expand nodes of the tree
+    # that are not included in the macros node
+    macros_el = _macros_el(root)
+    if macros_el is not None:
+        macros_copy = copy(macros_el)
+        macros_el.clear()
+    else:
+        macros_copy = None
+
     # Collect tokens
-    tokens = _macros_of_type(root, 'token', lambda el: el.text or '')
+    tokens = _macros_of_type(macros_copy, 'token', lambda el: el.text or '')
     tokens = expand_nested_tokens(tokens)
 
     # Expand xml macros
-    macro_dict = _macros_of_type(root, 'xml', lambda el: XmlMacroDef(el))
+    macro_dict = _macros_of_type(macros_copy, 'xml', lambda el: XmlMacroDef(el))
     _expand_macros([root], macro_dict, tokens)
+
+    # readd the stashed children of the macros node
+    # TODO is this this really necesary? Since macro nodes are removed anyway just below.
+    if macros_copy is not None:
+        _xml_set_children(macros_el, list(macros_copy))
+
     for el in root.xpath('//macro'):
         if el.get('type') != 'template':
             # Only keep template macros
@@ -43,7 +60,8 @@ def template_macro_params(root):
     with these.
     """
     param_dict = {}
-    macro_dict = _macros_of_type(root, 'template', lambda el: el.text)
+    macros_el = _macros_el(root)
+    macro_dict = _macros_of_type(macros_el, 'template', lambda el: el.text)
     for key, value in macro_dict.items():
         param_dict[key] = value
     return param_dict
@@ -63,6 +81,10 @@ def imported_macro_paths(root):
 
 
 def _import_macros(root, path):
+    """
+    root the parsed XML tree
+    path the path to the main xml document
+    """
     xml_base_dir = os.path.dirname(path)
     macros_el = _macros_el(root)
     if macros_el is not None:
@@ -75,16 +97,14 @@ def _macros_el(root):
     return root.find('macros')
 
 
-def _macros_of_type(root, type, el_func):
-    macros_el = root.find('macros')
-    macro_dict = {}
-    if macros_el is not None:
-        macro_els = macros_el.findall('macro')
-        filtered_els = [(macro_el.get("name"), el_func(macro_el))
-                        for macro_el in macro_els
-                        if macro_el.get('type') == type]
-        macro_dict = dict(filtered_els)
-    return macro_dict
+def _macros_of_type(macros_el, type, el_func):
+    if macros_el is None:
+        return {}
+    macro_els = macros_el.findall('macro')
+    filtered_els = [(macro_el.get("name"), el_func(macro_el))
+                    for macro_el in macro_els
+                    if macro_el.get('type') == type]
+    return dict(filtered_els)
 
 
 def expand_nested_tokens(tokens):
@@ -106,6 +126,11 @@ def _expand_tokens(elements, tokens):
 
 
 def _expand_tokens_for_el(element, tokens):
+    """
+    expand tokens in element and (recursively) in its children
+    replacements of text attributes and attribute values are
+    possible
+    """
     value = element.text
     if value:
         new_value = _expand_tokens_str(element.text, tokens)
@@ -115,6 +140,11 @@ def _expand_tokens_for_el(element, tokens):
         new_value = _expand_tokens_str(value, tokens)
         if not (new_value is value):
             element.attrib[key] = new_value
+        new_key = _expand_tokens_str(key, tokens)
+        if not (new_key is key):
+            element.attrib[new_key] = element.attrib[key]
+            del element.attrib[key]
+    # recursively expand in childrens
     _expand_tokens(list(element), tokens)
 
 
@@ -125,7 +155,7 @@ def _expand_tokens_str(s, tokens):
     return s
 
 
-def _expand_macros(elements, macros, tokens):
+def _expand_macros(elements, macros, tokens, visited=None):
     if not macros and not tokens:
         return
 
@@ -134,48 +164,60 @@ def _expand_macros(elements, macros, tokens):
             expand_el = element.find('.//expand')
             if expand_el is None:
                 break
-            _expand_macro(element, expand_el, macros, tokens)
+            if visited is None:
+                v = []
+            else:
+                v = visited
+            _expand_macro(expand_el, macros, tokens, v)
 
 
-def _expand_macro(element, expand_el, macros, tokens):
+def _expand_macro(expand_el, macros, tokens, visited):
     macro_name = expand_el.get('macro')
     assert macro_name is not None, "Attempted to expand macro with no 'macro' attribute defined."
+
+    # check for cycles in the nested macro expansion
+    assert macro_name not in visited, f"Cycle in nested macros: already expanded {visited} can't expand '{macro_name}' again"
+    visited.append(macro_name)
+
     assert macro_name in macros, f"No macro named {macro_name} found, known macros are {', '.join(macros.keys())}."
     macro_def = macros[macro_name]
-    expanded_elements = deepcopy(macro_def.elements)
-
+    expanded_elements = deepcopy(macro_def.element)
     _expand_yield_statements(expanded_elements, expand_el)
 
-    # Recursively expand contained macros.
-    _expand_macros(expanded_elements, macros, tokens)
     macro_tokens = macro_def.macro_tokens(expand_el)
     if macro_tokens:
         _expand_tokens(expanded_elements, macro_tokens)
 
+    # Recursively expand contained macros.
+    _expand_macros(expanded_elements, macros, tokens, visited)
     _xml_replace(expand_el, expanded_elements)
+    del visited[-1]
 
 
 def _expand_yield_statements(macro_def, expand_el):
-    yield_els = [yield_el for macro_def_el in macro_def for yield_el in macro_def_el.findall('.//yield')]
-
-    expand_el_children = list(expand_el)
+    """
+    Modifies the macro_def element by replacing
+    1. all named yield tags by the content of the corresponding token tags
+       - token tags need to be direct children of the expand
+       - processed in order of definition of the token tags
+    2. all unnamed yield tags by the non-token children of the expand tag
+    """
+    # replace named yields
+    for token_el in expand_el.findall('./token'):
+        name = token_el.attrib.get("name", None)
+        assert name is not None, "Found unnamed token" + str(token_el.attrib)
+        yield_els = [yield_el for yield_el in macro_def.findall(f".//yield[@name='{name}']")]
+        assert len(yield_els) > 0, f"No named yield found for named token {name}"
+        token_el_children = list(token_el)
+        for yield_el in yield_els:
+            _xml_replace(yield_el, token_el_children)
 
+    # replace unnamed yields
+    yield_els = [yield_el for yield_el in macro_def.findall('.//yield')]
+    expand_el_children = [c for c in expand_el if c.tag != "token"]
     for yield_el in yield_els:
         _xml_replace(yield_el, expand_el_children)
 
-    # Replace yields at the top level of a macro, seems hacky approach
-    replace_yield = True
-    while replace_yield:
-        for i, macro_def_el in enumerate(macro_def):
-            if macro_def_el.tag == "yield":
-                for target in expand_el_children:
-                    i += 1
-                    macro_def.insert(i, target)
-                macro_def.remove(macro_def_el)
-                continue
-
-        replace_yield = False
-
 
 def _load_macros(macros_el, xml_base_dir):
     macros = []
@@ -225,7 +267,6 @@ def _load_imported_macros(macros_el, xml_base_dir):
         file_macros, current_macro_paths = _load_macro_file(import_path, xml_base_dir)
         macros.extend(file_macros)
         macro_paths.extend(current_macro_paths)
-
     return macros, macro_paths
 
 
@@ -270,9 +311,25 @@ def _xml_replace(query, targets):
 
 
 class XmlMacroDef:
+    """
+    representation of a (Galaxy) XML macro
 
+    stores the root element of the macro and the parameters.
+    each parameter is represented as pair containing
+    - the quote character, default '@'
+    - parameter name
+
+    parameter names can be given as comma separated list using the
+    `token` attribute or as attributes `token_XXX` (where `XXX` is the name).
+    The former option should be used to specify required attributes of the
+    macro and the latter for optional attributes if the macro (the value of
+    `token_XXX is used as default value).
+
+    TODO: `token_quote` forbids `"quote"` as character name of optional
+    parameters
+    """
     def __init__(self, el):
-        self.elements = list(el)
+        self.element = el
         parameters = {}
         tokens = []
         token_quote = "@"
@@ -290,12 +347,17 @@ class XmlMacroDef:
         self.parameters = parameters
 
     def macro_tokens(self, expand_el):
+        """
+        get a dictionary mapping token names to values. The names are the
+        parameter names surrounded by the quote character. Values are taken
+        from the expand_el if absent default values of optional parameters are
+        used.
+        """
         tokens = {}
         for key, (wrap_char, default_val) in self.parameters.items():
             token_value = expand_el.attrib.get(key, default_val)
             if token_value is REQUIRED_PARAMETER:
-                message = "Failed to expand macro - missing required parameter [%s]."
-                raise ValueError(message % key)
+                raise ValueError(f"Failed to expand macro - missing required parameter [{key}].")
             token_name = f"{wrap_char}{key.upper()}{wrap_char}"
             tokens[token_name] = token_value
         return tokens

test/unit/tool_util/test_tool_loader.py

@@ -4,34 +4,45 @@ from tempfile import mkdtemp
 
 from galaxy.tool_util.loader import load_tool, template_macro_params
 from galaxy.tool_util.unittest_utils.sample_data import SIMPLE_MACRO, SIMPLE_TOOL_WITH_MACRO
-from galaxy.util import parse_xml
+from galaxy.util import parse_xml, xml_to_string
 
 
-def test_loader():
+class TestToolDirectory:
+    __test__ = False  # Prevent pytest from discovering this class (issue #12071)
 
-    class TestToolDirectory:
-        __test__ = False  # Prevent pytest from discovering this class (issue #12071)
+    def __init__(self):
+        self.temp_directory = mkdtemp()
 
-        def __init__(self):
-            self.temp_directory = mkdtemp()
+    def __enter__(self):
+        return self
 
-        def __enter__(self):
-            return self
+    def __exit__(self, type, value, tb):
+        rmtree(self.temp_directory)
 
-        def __exit__(self, type, value, tb):
-            rmtree(self.temp_directory)
+    def write(self, contents, name="tool.xml"):
+        open(os.path.join(self.temp_directory, name), "w").write(contents)
 
-        def write(self, contents, name="tool.xml"):
-            open(os.path.join(self.temp_directory, name), "w").write(contents)
+    def load(self, name="tool.xml", preprocess=True):
+        path = os.path.join(self.temp_directory, name)
+        if preprocess:
+            return load_tool(path)
+        else:
+            return parse_xml(path)
 
-        def load(self, name="tool.xml", preprocess=True):
-            path = os.path.join(self.temp_directory, name)
-            if preprocess:
-                return load_tool(path)
-            else:
-                return parse_xml(path)
 
-    # Test simple macro replacement.
+def test_no_macros():
+    """
+    Test tool loaded in absence of a macros node.
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('<tool/>')
+        tool_dir.load(preprocess=True)
+
+
+def test_loader_simple():
+    """
+    Test simple macro replacement.
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -47,7 +58,11 @@ def test_loader():
         xml = tool_dir.load(preprocess=True)
         assert xml.find("inputs") is not None
 
-    # Test importing macros from external files
+
+def test_loader_external():
+    """
+    Test importing macros from external files
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write(SIMPLE_TOOL_WITH_MACRO)
 
@@ -57,6 +72,8 @@ def test_loader():
         xml = tool_dir.load(preprocess=True)
         assert xml.find("inputs") is not None
 
+
+def test_loader_unnamed_yield():
     # Test macros with unnamed yield statements.
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
@@ -84,12 +101,15 @@ def test_loader():
     </macros>
 </tool>''')
         xml = tool_dir.load()
-        assert xml.findall("inputs")[0].find("input").get("name") == "first_input"
-        assert xml.findall("inputs")[1].find("input").get("name") == "second_input"
-        assert xml.findall("inputs")[2].find("input").get("name") == "third_input"
+        assert xml.find("/inputs[1]/input").get("name") == "first_input"
+        assert xml.find("/inputs[2]/input").get("name") == "second_input"
+        assert xml.find("/inputs[3]/input").get("name") == "third_input"
 
-    # Test nested macro with yield statements
 
+def test_loader_unnamed_yield_nested():
+    """
+    Test nested macro with yield statements
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write("""
 <tool>
@@ -111,13 +131,22 @@ def test_loader():
         </macro>
     </macros>
     <inputs>
-        <expand macro="single_or_paired_general"></expand>
+        <expand macro="single_or_paired_general">
+            <param name="test"/>
+        </expand>
     </inputs>
 </tool>
 """)
         xml = tool_dir.load()
+        # assert the both yields in the inner macro (paired_options) are expanded
+        assert xml.find('/inputs/conditional[@name="library"]/when[@value="paired"]/param[@name="test"]') is not None
+        assert xml.find('/inputs/conditional[@name="library"]/when[@value="paired_collection"]/param[@name="test"]') is not None
 
-    # Test recursive macro applications.
+
+def test_loader_recursive():
+    """
+    Test recursive macro applications.
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -138,11 +167,15 @@ def test_loader():
     </macros>
 </tool>''')
         xml = tool_dir.load()
-        assert xml.find("inputs").findall("input")[0].get("name") == "first_input"
-        assert xml.find("inputs").findall("input")[1].get("name") == "second_input"
-        assert xml.find("inputs").findall("input")[2].get("name") == "third_input"
+        assert xml.find("/inputs/input[1]").get("name") == "first_input"
+        assert xml.find("/inputs/input[2]").get("name") == "second_input"
+        assert xml.find("/inputs/input[3]").get("name") == "third_input"
 
-    # Test recursive macro applications.
+
+def test_loader_recursive2():
+    """
+    Test recursive macro applications.
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -166,10 +199,15 @@ def test_loader():
     </macros>
 </tool>''')
         xml = tool_dir.load()
-        assert xml.find("inputs").findall("input")[0].get("name") == "first_input"
-        assert xml.find("inputs").findall("input")[1].get("name") == "second_input"
-        assert xml.find("inputs").findall("input")[2].get("name") == "third_input"
+        assert xml.find("/inputs/input[1]").get("name") == "first_input"
+        assert xml.find("/inputs/input[2]").get("name") == "second_input"
+        assert xml.find("/inputs/input[3]").get("name") == "third_input"
 
+
+def test_loader_toplevel_yield():
+    """
+    test expansion of top level (ie child of <macro>) yields
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool id="issue_647">
@@ -186,10 +224,14 @@ def test_loader():
     </inputs>
 </tool>''')
         xml = tool_dir.load()
-        assert xml.find("inputs").findall("param")[0].get("name") == "a1"
-        assert xml.find("inputs").findall("param")[1].get("name") == "b"
+        assert xml.find("/inputs/param[1]").get("name") == "a1"
+        assert xml.find("/inputs/param[2]").get("name") == "b"
 
-    # Test <xml> is shortcut for macro type="xml"
+
+def test_loader_shortcut():
+    """
+    Test <xml> is shortcut for macro type="xml"
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -203,6 +245,8 @@ def test_loader():
         xml = tool_dir.load()
         assert xml.find("inputs") is not None
 
+
+def test_loader_template():
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -218,6 +262,8 @@ def test_loader():
         params_dict = template_macro_params(xml.getroot())
         assert params_dict['tool_params'] == "-a 1 -b 2"
 
+
+def test_loader_token_text():
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -234,6 +280,76 @@ def test_loader():
         help_el = xml.find("help")
         assert help_el.text == "The citation.", help_el.text
 
+
+def test_loader_token_nested():
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <token name="@WIZARD@">Ab@HELPER@ra</token>
+        <token name="@HELPER@">ra@WITCH@ab</token>
+        <token name="@WITCH@">cad</token>
+    </macros>
+    <help>@WIZARD@</help>
+    <another>
+        <tag />
+    </another>
+</tool>
+''')
+        xml = tool_dir.load()
+        help_el = xml.find("help")
+        assert help_el.text == "Abracadabra", help_el.text
+
+
+def test_loader_token_cycle():
+    """
+    test if cycles in nested tokens are detected
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <token name="@A@">a @ROSE@</token>
+        <token name="@ROSE@">rose @IS@</token>
+        <token name="@IS@">a @A@</token>
+    </macros>
+    <help>@A@</help>
+    <another>
+        <tag />
+    </another>
+</tool>
+''')
+        try:
+            tool_dir.load()
+        except Exception as e:
+            assert str(e) == "Token '@IS@' cannot contain itself"
+        else:
+            raise AssertionError("Cycle not detected, but then we are doomed anyway")
+
+
+def test_loader_token_attribute_name():
+    """
+    test the repacement of an attribute name by a token
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <token name="__ATTR_NAME">name</token>
+    </macros>
+    <another>
+        <tag __ATTR_NAME="blah" />
+    </another>
+</tool>
+''')
+        xml = tool_dir.load()
+        assert xml.find('/another/tag[@name="blah"]') is not None
+
+
+def test_loader_token_attribute_value():
+    """
+    test the repacement of an attribute value by a token
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -246,10 +362,10 @@ def test_loader():
 </tool>
 ''')
         xml = tool_dir.load()
-        tag_el = xml.find("another").find("tag")
-        value = tag_el.get('value')
-        assert value == "The value.", value
+        assert xml.find('/another/tag[@value="The value."]') is not None
 
+
+def test_loader_token_empty():
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -262,11 +378,13 @@ def test_loader():
 </tool>
 ''')
         xml = tool_dir.load()
-        tag_el = xml.find("another").find("tag")
-        value = tag_el.get('value')
-        assert value == "", value
+        assert xml.find('/another/tag[@value=""]') is not None
 
-    # Test macros XML macros with $$ expansions in attributes
+
+def test_loader_macro_token_quote():
+    """
+    Test macros XML macros with $$ expansions in attributes
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -287,7 +405,11 @@ def test_loader():
         assert input_els[1].attrib["type"] == "the type is my awesome"
         assert input_els[2].attrib["type"] == "the type is doggo"
 
-    # Test macros XML macros with @ expansions in text
+
+def test_loader_macro_token():
+    """
+    Test macros XML macros with @ expansions in text
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -308,7 +430,11 @@ def test_loader():
         assert input_els[1].text == "world"
         assert input_els[2].text == "the_default"
 
-    # Test macros XML macros with @ expansions and recursive
+
+def test_loader_macro_token_recursive():
+    """
+    Test macros XML macros with @ expansions and recursive
+    """
     with TestToolDirectory() as tool_dir:
         tool_dir.write('''
 <tool>
@@ -331,3 +457,217 @@ def test_loader():
         assert input_els[0].find("cow").text == "hello"
         assert input_els[1].find("cow").text == "world"
         assert input_els[2].find("cow").text == "the_default"
+
+
+def test_loader_macro_named_yield():
+    """
+    test expansion of named and unnamed yield
+    - named yields are replaced by content of the corresponding token
+    - unnamed yields are replaced by all non-token elements of the expand tag
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <xml name="test">
+            <A>
+                <yield/>
+            </A>
+            <yield name="token1"/>
+            <B>
+                <yield/>
+                <yield name="token2"/>
+            </B>
+        </xml>
+    </macros>
+    <expand macro="test">
+        <token name="token1">
+            <content_of_token1/>
+            <more_content_of_token1/>
+        </token>
+        <sub_of_expand_1/>
+        <token name="token2">
+            <content_of_token2/>
+            <more_content_of_token2/>
+        </token>
+        <sub_of_expand_2/>
+    </expand>
+</tool>
+''')
+        xml = tool_dir.load()
+        assert xml_to_string(xml, pretty=True) == '''<?xml version="1.0" ?>
+<tool>
+    <macros/>
+    <A>
+        <sub_of_expand_1/>
+        <sub_of_expand_2/>
+    </A>
+    <content_of_token1/>
+    <more_content_of_token1/>
+    <B>
+        <sub_of_expand_1/>
+        <sub_of_expand_2/>
+        <content_of_token2/>
+        <more_content_of_token2/>
+    </B>
+</tool>'''
+
+
+def test_loader_macro_multiple_toplevel_yield():
+    """
+    test replacement of multiple top level yield
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <xml name="test">
+            <blah/>
+            <yield/>
+            <blah>
+                <yield name="token1"/>
+            </blah>
+            <yield name="token2"/>
+        </xml>
+    </macros>
+    <expand macro="test">
+        <token name="token1">
+            <content_of_token1/>
+            <more_content_of_token1/>
+        </token>
+        <sub_of_expand_1/>
+        <token name="token2">
+            <content_of_token2/>
+            <more_content_of_token2/>
+        </token>
+        <sub_of_expand_2/>
+    </expand>
+</tool>
+''')
+        xml = tool_dir.load()
+        assert xml_to_string(xml, pretty=True) == '''<?xml version="1.0" ?>
+<tool>
+    <macros/>
+    <blah/>
+    <sub_of_expand_1/>
+    <sub_of_expand_2/>
+    <blah>
+        <content_of_token1/>
+        <more_content_of_token1/>
+    </blah>
+    <content_of_token2/>
+    <more_content_of_token2/>
+</tool>'''
+
+
+def test_loader_macro_recursive_named_yield():
+    """
+    test 'recursive' replacement with named yields
+    since named yields are processed in order of the definition of the
+    corresponding tokens:
+    - replacing yield for token1 introduces yield for token2
+    - replacing yield for token2 introduced unnamed yield
+    - replacing unnamed yield gives the only non-token element of the expand
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <xml name="test">
+            <A>
+                <yield name="token1"/>
+            </A>
+        </xml>
+    </macros>
+    <expand macro="test">
+        <token name="token1">
+            <T1>
+                <yield name="token2"/>
+            </T1>
+        </token>
+        <token name="token2">
+            <T2>
+                <yield/>
+            </T2>
+        </token>
+        <T/>
+    </expand>
+</tool>''')
+
+        xml = tool_dir.load()
+        assert xml_to_string(xml, pretty=True) == '''<?xml version="1.0" ?>
+<tool>
+    <macros/>
+    <A>
+        <T1>
+            <T2>
+                <T/>
+            </T2>
+        </T1>
+    </A>
+</tool>'''
+
+
+def test_loader_specify_nested_macro_by_token():
+    """
+    test if a nested macro can have a nested
+    macro specifying the macro name via a token
+    of the outer macro
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <import>external.xml</import>
+    </macros>
+    <expand macro="testp" token_name="a"/>
+    <expand macro="testp" token_name="b"/>
+</tool>''')
+        tool_dir.write('''
+<macros>
+    <xml name="a">
+        <A/>
+    </xml>
+    <xml name="b">
+        <B/>
+    </xml>
+    <xml name="testp" tokens="token_name">
+        <expand macro="@TOKEN_NAME@"/>
+    </xml>
+</macros>
+''', name="external.xml")
+
+        xml = tool_dir.load()
+        assert xml_to_string(xml, pretty=True) == '''<?xml version="1.0" ?>
+<tool>
+    <macros/>
+    <A/>
+    <B/>
+</tool>'''
+
+
+def test_loader_circular_macros():
+    """
+    check the cycles in nested macros are detected
+    """
+    with TestToolDirectory() as tool_dir:
+        tool_dir.write('''
+<tool>
+    <macros>
+        <xml name="a">
+            <A>
+                <expand macro="b"/>
+            </A>
+        </xml>
+        <xml name="b">
+            <B>
+                <expand macro="a"/>
+            </B>
+        </xml>
+    </macros>
+    <expand macro="a"/>
+</tool>''')
+        try:
+            tool_dir.load()
+        except AssertionError as a:
+            assert str(a) == "Cycle in nested macros: already expanded ['a', 'b'] can't expand 'a' again"