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

Behat xpath union is too greedy

XMLWordPrintable

      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

            dobedobedoh Andrew Lyons
            dobedobedoh Andrew Lyons
            Simey Lameze Simey Lameze
            Eloy Lafuente (stronk7) Eloy Lafuente (stronk7)
            CiBoT CiBoT
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved:

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

                  Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.