Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-66457

Behat xpath union is too greedy

    XMLWordPrintable

    Details

      Description

      I'm trying to write a step to select a repeated field where each repeat is in a different fieldset.
      i.e.

      fieldset: Whole forum grade
      field: Grade to pass
       
      fieldset: Ratings
      field: Grade to pass
      

      I first considered adding the name of the fieldset to the langstring, but this seems wrong as I suspect that the name of the fieldset will also be read out by a screenreader so context is maintained (not yet verified).

      So I've been looking to amend the find_field step to support the standard > notation, i.e.

      I set the "Ratings > Grade to pass" field to "80"
      

      I then explode on the ">", use this to find the "Ratings" fieldset, and then look for the "Grade to pass" within that fieldset as you would expect.

      That is relatively easy and should work, however the way that the optional container $node is specified takes the xpath for the target ("Grade to pass" "field") and explodes it on the pipe character ("|") which is an xpath UNION operator.
      It then prefixes each component with the xpath of the container.

      All good in theory, except that the pipe operator is a perfectly valid operator in the middle of an xpath too.

      For example:

      //*
      [self::input[translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'radio' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'checkbox' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'file'] | self::select][not(translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'submit' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'image' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'button' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'reset' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'hidden')][((./@id = 'Aggregate type' or ./@name = 'Aggregate type') or ./@id = //label[contains(normalize-space(string(.)), 'Aggregate type')]/@for)]
      

      When prefixed with:

      (//html/.//fieldset
      [(./@id = 'Ratings' or .//legend[contains(normalize-space(string(.)), 'Ratings')])])[1]
      

      Turns to:

      (//html/.//fieldset
      [(./@id = 'Ratings' or .//legend[contains(normalize-space(string(.)), 'Ratings')])])[1]//*
      [self::input[translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'radio' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'checkbox' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'file']
       
      (//html/.//fieldset
      [(./@id = 'Ratings' or .//legend[contains(normalize-space(string(.)), 'Ratings')])])[1]/self::select][not(translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'submit' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'image' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'button' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'reset' or translate(./@type, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = 'hidden')][((./@id = 'Aggregate type' or ./@name = 'Aggregate type') or ./@id = //label[contains(normalize-space(string(.)), 'Aggregate type')]/@for)]
      

      Note the section:

      (//html/.//fieldset
      [(./@id = 'Ratings' or .//legend[contains(normalize-space(string(.)), 'Ratings')])])[1]/self::select]
      

      To clarify further...

      (
          //html/.//fieldset
              [(
                  ./@id = 'Ratings'
              or
                  .//legend[contains('Ratings')]
              )]
      )[1]
      /self::select]
      

      That final /self::select] does not have a matching [ because in the original xpath it was paired before the UNION operator.
      So it's an invalid XPath. It's valid to specify a UNION in the middle of a pair of [] brackets in this way, butthe xpath prefix we were performing was not intelligent enough to know how to handle it.

      After spending several hours trying to find a solution to my behat test failures, I have found the solution... just call findAll on the container $node.
      The Mink driver already makes use of Behat\Mink\Selector\Xpath\Manipulator to prepend the node to the container, and that manipulator is aware of the union operator and how to do it properly.

      This feature was introduced in Mink 1.7.1 in 2011 but we started with Mink 1.4 which did not contain this feature.

      The upstream implementation is almost identical to the one I am removing... the key difference between the two is that the upstream implementation uses a preg_match with lookahead to ensure that it only picks up pipes which are not in brackets (which was the bug I was experiencing).
      https://github.com/minkphp/Mink/blob/316210b1c0b048f62c59e1ceb84971d499e8ac66/src/Behat/Mink/Element/Element.php#L191

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                dobedobedoh Andrew Nicols
                Reporter:
                dobedobedoh Andrew Nicols
                Peer reviewer:
                Simey Lameze
                Integrator:
                Eloy Lafuente (stronk7)
                Tester:
                CiBoT
                Participants:
                Component watchers:
                Andrew Nicols, Mathew May, Michael Hawkins, Shamim Rezaie, Simey Lameze
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/19

                  Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 4 hours, 52 minutes
                  4h 52m