Moodle
  1. Moodle
  2. MDL-43713

Support multiple option select boxes in behat

    Details

    • Testing Instructions:
      Hide

      1) Copy the attached feature file to the dirroot of the site.

      2) Execute with firefox, chrome, ie, phantomjs. It covers all the cases implemented by this issue. Should end with 0 failures. Note there are failures with safari, in "verify that every set operation clears previous one", but it's in a case not used in core (multiple editions of multi-select without saving the form).

      /vendor/bin/behat --config path/to/your/config \
          /full/path/to/the/attached/feature/file/multiple_select.feature
      

      3) Run the whole behat suite against all browsers. There should not be any regression in existing uses of select boxes.

      That's all. Thanks!

      Show
      1) Copy the attached feature file to the dirroot of the site. 2) Execute with firefox, chrome, ie, phantomjs. It covers all the cases implemented by this issue. Should end with 0 failures. Note there are failures with safari, in "verify that every set operation clears previous one", but it's in a case not used in core (multiple editions of multi-select without saving the form). /vendor/bin/behat --config path/to/your/config \ /full/path/to/the/attached/feature/file/multiple_select.feature 3) Run the whole behat suite against all browsers. There should not be any regression in existing uses of select boxes. That's all. Thanks!
    • Affected Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Was working on when I decided to select 2 values from a multiple list box. And then realized it was impossible. I tried with all these:

      • I fill the moodle form with (table)
      • I fill in "FIELD_STRING" with "VALUE_STRING"
      • I select "OPTION_STRING" from "SELECT_STRING"

      And all them, when selecting the second option, unselects the previous one.

      Looking to this now....

        Gliffy Diagrams

        1. multiple_select.feature
          11 kB
          Eloy Lafuente (stronk7)

          Issue Links

            Activity

            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Done, I think I've broken it. Summary (form commit message):

            This patch implements:

            1) Normalization of options. Before the patch options
            in a select were being returned as "op1 op2 op3" by selenium
            and "op1op2op3" by goutte. With the patch, those lists
            are always returned like "op1, op2, op3".

            2) Support for selecting multiple options. Before the patch
            only one option was selected and a new selection was cleaning the
            previous one. With the patch it's possible to pass "op1, op2" in
            these steps:

            • I fill the moodle form with (table)
            • I select "OPTION_STRING" from "SELECT_STRING"

            3) Ability to match multiple options. Before the
            patch, matching of multiple options was really random, now every
            passed option ("opt1, opt2") is individually verified. It applies
            to these 2 steps:

            • the "ELEMENT" select box should contain "OPTIONS"
            • the "ELEMENT" select box should not contain "OPTIONS"

            4) Two new steps able to verify if a form has some options selected or no:

            • the "ELEMENT" select box should contain "OPTIONS" selected
            • the "ELEMENT" select box should contain "OPTIONS" not selected

            Observations:

            A- There are some noticeable pauses when handling select boxes. Do we really need all them?

            B- I've performed a call from a step-definition function to another one (delegating half the checks to it). Is that correct? Or should I be calling the step instead? See https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-c025fd885572803711cda8769f33c643R427

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Done, I think I've broken it. Summary (form commit message): This patch implements: 1) Normalization of options. Before the patch options in a select were being returned as "op1 op2 op3" by selenium and "op1op2op3" by goutte. With the patch, those lists are always returned like "op1, op2, op3". 2) Support for selecting multiple options. Before the patch only one option was selected and a new selection was cleaning the previous one. With the patch it's possible to pass "op1, op2" in these steps: I fill the moodle form with (table) I select "OPTION_STRING" from "SELECT_STRING" 3) Ability to match multiple options. Before the patch, matching of multiple options was really random, now every passed option ("opt1, opt2") is individually verified. It applies to these 2 steps: the "ELEMENT" select box should contain "OPTIONS" the "ELEMENT" select box should not contain "OPTIONS" 4) Two new steps able to verify if a form has some options selected or no: the "ELEMENT" select box should contain "OPTIONS" selected the "ELEMENT" select box should contain "OPTIONS" not selected Observations: A- There are some noticeable pauses when handling select boxes. Do we really need all them? B- I've performed a call from a step-definition function to another one (delegating half the checks to it). Is that correct? Or should I be calling the step instead? See https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-c025fd885572803711cda8769f33c643R427
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Seems to be working here (feature passed ok) so, while I'm performing a complete run... sending this to peer-review.

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Seems to be working here (feature passed ok) so, while I'm performing a complete run... sending this to peer-review.
            Hide
            CiBoT added a comment -

            Results for MDL-43713

            • Remote repository: git://github.com/stronk7/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43713 Remote repository: git://github.com/stronk7/moodle.git Remote branch MDL-43713 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/560 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/560/artifact/work/smurf.html Remote branch MDL-43713 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/561 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/561/artifact/work/smurf.html
            Hide
            David Monllaó added a comment - - edited

            Thanks for creating the issue and working on it, and congrats, I have to admit it, it is a painful issue that set_value() is the most dodgy method in all the behat framework as all drivers behave differently when selecting them and the JS events triggered are not always triggered from the same point depending on the driver... Anyway, the review:

            1) As commented in the dev chat admin/tool/behat/tests/behat/basic_actions.feature purpose is to test some of the existing step definitions, feel free to mix multiple_select.feature's use cases in there or to expand the wiki test you are writing to cover more cases (it would be a way to avoid testing steps instead of testing functionality that is what we want)

            2) NodeElement::hasAttribute() sends 1 xpath query, would be better to set a $multiple = $this->field->hasAttribute('multiple') rather that 2 queries as this method is widely used and it is already quite expensive to call

            3) I don't like the comma as a separator as it is a common character and can be part of an option string (I'm thinking of mod_quiz options for example) probably some special chars combination ($$$ or whatever + MDocs info + info in the step definitions description that allows it) would be better, also we could generalize that chars combination as behat_base::separator, we will probably need other "separations" in other cases.

            4) IMO https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L241 step description should include what to do when dealing with select multiple as it is what it is currently used to check selected values in select fields

            5) I understand that #1 (commit msg) is https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-73977cc9bf283820a3c7d0b6759b149fR173 isn't it?

            6) Same about #1 (commit msg) and https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L305, it is done like this to unify Goutte & Selenium returned values isn't it? In this case, can you please add a comment about it?

            7) https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L281 The step definition description should be updated according to the new allowed options. Also https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L287

            8) (Picky point) As you know I'm not the best reference about English grammar, but https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-c025fd885572803711cda8769f33c643R392 does not sound specially good to me, maybe "$option is not a $select select box selected option", bufff, well, I don't know, it neither sounds good to me

            About the observations:
            A) Yes, I added them because they were failures due to unfinished AJAX requests when selecting elements and AJAX requests were triggered (some drivers)
            B) It is ok like you have done it, it is a method and can be perfectly called, you can only call the step at the end of the step, as they are the return and they are executed once the current step is finished https://github.com/moodlehq/moodle-behat-extension/blob/master/src/Moodle/BehatExtension/Tester/MoodleStepTester.php#L209

            Note that during peer review I haven't ran the testing instructions.

            (Edited: to number points for easier reference)

            Show
            David Monllaó added a comment - - edited Thanks for creating the issue and working on it, and congrats, I have to admit it, it is a painful issue that set_value() is the most dodgy method in all the behat framework as all drivers behave differently when selecting them and the JS events triggered are not always triggered from the same point depending on the driver... Anyway, the review: 1) As commented in the dev chat admin/tool/behat/tests/behat/basic_actions.feature purpose is to test some of the existing step definitions, feel free to mix multiple_select.feature's use cases in there or to expand the wiki test you are writing to cover more cases (it would be a way to avoid testing steps instead of testing functionality that is what we want) 2) NodeElement::hasAttribute() sends 1 xpath query, would be better to set a $multiple = $this->field->hasAttribute('multiple') rather that 2 queries as this method is widely used and it is already quite expensive to call 3) I don't like the comma as a separator as it is a common character and can be part of an option string (I'm thinking of mod_quiz options for example) probably some special chars combination ($$$ or whatever + MDocs info + info in the step definitions description that allows it) would be better, also we could generalize that chars combination as behat_base::separator, we will probably need other "separations" in other cases. 4) IMO https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L241 step description should include what to do when dealing with select multiple as it is what it is currently used to check selected values in select fields 5) I understand that #1 (commit msg) is https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-73977cc9bf283820a3c7d0b6759b149fR173 isn't it? 6) Same about #1 (commit msg) and https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L305 , it is done like this to unify Goutte & Selenium returned values isn't it? In this case, can you please add a comment about it? 7) https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L281 The step definition description should be updated according to the new allowed options. Also https://github.com/stronk7/moodle/blob/0245906c9ea2faa6f89aae3abc09fb205e6bf829/lib/tests/behat/behat_forms.php#L287 8) (Picky point) As you know I'm not the best reference about English grammar, but https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-c025fd885572803711cda8769f33c643R392 does not sound specially good to me, maybe "$option is not a $select select box selected option", bufff, well, I don't know, it neither sounds good to me About the observations: A) Yes, I added them because they were failures due to unfinished AJAX requests when selecting elements and AJAX requests were triggered (some drivers) B) It is ok like you have done it, it is a method and can be perfectly called, you can only call the step at the end of the step, as they are the return and they are executed once the current step is finished https://github.com/moodlehq/moodle-behat-extension/blob/master/src/Moodle/BehatExtension/Tester/MoodleStepTester.php#L209 Note that during peer review I haven't ran the testing instructions. (Edited: to number points for easier reference)
            Hide
            David Monllaó added a comment -

            Ups, extra minor issue, https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-c025fd885572803711cda8769f33c643R296, the explode() can be outside the foreach, otherwise it is executed at each iteration IIRC

            Show
            David Monllaó added a comment - Ups, extra minor issue, https://github.com/stronk7/moodle/compare/master...MDL-43713#diff-c025fd885572803711cda8769f33c643R296 , the explode() can be outside the foreach, otherwise it is executed at each iteration IIRC
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Side comment, after executing the whole suite, this has caused problems in "Importing of groups and groupings":

            01. Field matching locator "'id_idnumber'" not found.
                In step `And the "id_idnumber" field should match "group-id-2" value'.                                   # behat_forms::the_field_should_match_value()
                From scenario `Import groups with idnumber when the user has proper permissions for the idnumber field'. # group/tests/behat/groups_import.feature:44
                Of feature `Importing of groups and groupings'.                                                          # group/tests/behat/groups_import.feature
             
            02. Field matching locator "'id_idnumber'" not found.
                In step `And the "id_idnumber" field should match "" value'.                                                       # behat_forms::the_field_should_match_value()
                From scenario `Import groups with idnumber when the user does not have proper permissions for the idnumber field'. # group/tests/behat/groups_import.feature:81
                Of feature `Importing of groups and groupings'.                                                                    # group/tests/behat/groups_import.feature
            

            The fix is simple. The first selectOption() must NOT be multiple. Only the next ones. Easy to fix. Will do and cover them with tests.

            Thanks for you review, will be digesting everything later today.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Side comment, after executing the whole suite, this has caused problems in "Importing of groups and groupings": 01. Field matching locator "'id_idnumber'" not found. In step `And the "id_idnumber" field should match "group-id-2" value'. # behat_forms::the_field_should_match_value() From scenario `Import groups with idnumber when the user has proper permissions for the idnumber field'. # group/tests/behat/groups_import.feature:44 Of feature `Importing of groups and groupings'. # group/tests/behat/groups_import.feature   02. Field matching locator "'id_idnumber'" not found. In step `And the "id_idnumber" field should match "" value'. # behat_forms::the_field_should_match_value() From scenario `Import groups with idnumber when the user does not have proper permissions for the idnumber field'. # group/tests/behat/groups_import.feature:81 Of feature `Importing of groups and groupings'. # group/tests/behat/groups_import.feature The fix is simple. The first selectOption() must NOT be multiple. Only the next ones. Easy to fix. Will do and cover them with tests. Thanks for you review, will be digesting everything later today. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Hi, some replies:

            1) Aha. I’ll add there a few tests, just to know it’s covered continuously. Also will add some more checks to the wiki one.

            2) You mean that, in the 2 new methods, instead of getting the node and looking for hasAttribute() in it.. y should be using the $field->hasAttribute() when available instead? Ok, will do.

            3) The separator problem is tricky thing. For sure, the most “natural” one is the comma. And it’s easy to write it in the features… also, note that the chances of problems are really small. It only affects to select boxes 1) being multiple and 2) using commas in the values.

            Perhaps we could keep the comma as separator and make mandatory to enclose the value with single quotes if it contains a comma?

            Or perhaps we could have a new step, I’ve seen it out there, named: “And I additionally select OPTION in ELEMENT” and make that one to only accept 1 option, not multiple.

            grrr… what a pity, the comma is really the best one. Adding any artificial separator will do the features ugly, grrr.

            4) Yeah, when looking to used stuff I saw both that step and also the “opposite” fill_field(). But I’m afraid, right now both are incomplete and don’t cover a lot of field types, basically only texts. For everything else (check boxes, selects…) there are other functions covering in detail. That’s the cause I only modified the specific ones (select box should [not] contain).

            So I’m not sure what should be done there. Because right now we have:

            setters (editors):

            • the whole table. “fill the moodle form with”
            • an individual field. “fill in FIELD with VALUE” (calling low level setValue() )
            • the specialized ones (for checks, radios, selects…)

            getters (checkers):

            • none at whole form level. would be great to have one checking multiple fields.
            • an individual field “the FIELD should match VALUE” (apparently not supporting all fields)
            • the specialized checkers

            For sure, ideally, the 1st ones should always be calling the individual one (that should support all fields). In fact, with those 2… surely we wouldn’t need the specialized ones anymore, keeping the “language” smaller and easier to remember. But right now, that’s not possible and out from the scope of this issue. I’d propose to move this discussion to another issue, to discuss about the API and how it should be, finally.

            5) yes, that’s the change transforming the unique (and different in selenium and goutte) text into a comma-separated list for all drivers. Just that.

            6) well, that one uses the same technique than 5) but applied to all the elements in the select box to get them also normalized. So I can compare every option to check ($optionsarr) against the normalized (comma-separated) list of elements in the select box. Before that normalization… that method was not reliable and could return true to non existing elements. Example, you had 2 elements, “a” and “b” and, if you checked if the element “ab” exists… it was returning true in goutte, for example, because the original getText() was returning that “ab”. I’d say it’s part of the 3) point of the commit message. make those 2 steps to support multiple options.

            7) Aha, will amend those to describe the new multiple possibilities. Also in the new functions.

            8) Hehe, I’m the first not liking the bloody names. But I finally used them to be 100% the same than the “The select box ELEMENT should contain OPTION” existing ones, just adding the “selected” and “not “selected” suffixes to qualify the search. Inventing a whole different name sounded like a worse solution than trying to follow current language. So… once again… I would keep this apart, perhaps we’ll end killing all these specialized functions if we are able to normalize the forms as commented in point 6). Of course I’m not either a specialist so I’m open to alternatives. Just explaining why I picked those horrible strings.

            9) The explode within the loop, will fix it!

            And that’s all, i’ll try to implement all the implementable, awaiting for your wise words about separators and friends. Will comment here once I’ve a new version ready.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Hi, some replies: 1) Aha. I’ll add there a few tests, just to know it’s covered continuously. Also will add some more checks to the wiki one. 2) You mean that, in the 2 new methods, instead of getting the node and looking for hasAttribute() in it.. y should be using the $field->hasAttribute() when available instead? Ok, will do. 3) The separator problem is tricky thing. For sure, the most “natural” one is the comma. And it’s easy to write it in the features… also, note that the chances of problems are really small. It only affects to select boxes 1) being multiple and 2) using commas in the values. Perhaps we could keep the comma as separator and make mandatory to enclose the value with single quotes if it contains a comma? Or perhaps we could have a new step, I’ve seen it out there, named: “And I additionally select OPTION in ELEMENT” and make that one to only accept 1 option, not multiple. grrr… what a pity, the comma is really the best one. Adding any artificial separator will do the features ugly, grrr. 4) Yeah, when looking to used stuff I saw both that step and also the “opposite” fill_field(). But I’m afraid, right now both are incomplete and don’t cover a lot of field types, basically only texts. For everything else (check boxes, selects…) there are other functions covering in detail. That’s the cause I only modified the specific ones (select box should [not] contain). So I’m not sure what should be done there. Because right now we have: setters (editors): the whole table. “fill the moodle form with” an individual field. “fill in FIELD with VALUE” (calling low level setValue() ) the specialized ones (for checks, radios, selects…) getters (checkers): none at whole form level. would be great to have one checking multiple fields. an individual field “the FIELD should match VALUE” (apparently not supporting all fields) the specialized checkers For sure, ideally, the 1st ones should always be calling the individual one (that should support all fields). In fact, with those 2… surely we wouldn’t need the specialized ones anymore, keeping the “language” smaller and easier to remember. But right now, that’s not possible and out from the scope of this issue. I’d propose to move this discussion to another issue, to discuss about the API and how it should be, finally. 5) yes, that’s the change transforming the unique (and different in selenium and goutte) text into a comma-separated list for all drivers. Just that. 6) well, that one uses the same technique than 5) but applied to all the elements in the select box to get them also normalized. So I can compare every option to check ($optionsarr) against the normalized (comma-separated) list of elements in the select box. Before that normalization… that method was not reliable and could return true to non existing elements. Example, you had 2 elements, “a” and “b” and, if you checked if the element “ab” exists… it was returning true in goutte, for example, because the original getText() was returning that “ab”. I’d say it’s part of the 3) point of the commit message. make those 2 steps to support multiple options. 7) Aha, will amend those to describe the new multiple possibilities. Also in the new functions. 8) Hehe, I’m the first not liking the bloody names. But I finally used them to be 100% the same than the “The select box ELEMENT should contain OPTION” existing ones, just adding the “selected” and “not “selected” suffixes to qualify the search. Inventing a whole different name sounded like a worse solution than trying to follow current language. So… once again… I would keep this apart, perhaps we’ll end killing all these specialized functions if we are able to normalize the forms as commented in point 6). Of course I’m not either a specialist so I’m open to alternatives. Just explaining why I picked those horrible strings. 9) The explode within the loop, will fix it! And that’s all, i’ll try to implement all the implementable, awaiting for your wise words about separators and friends. Will comment here once I’ve a new version ready. Ciao
            Hide
            David Monllaó added a comment -

            2) Sorry, I didn't explain it properly, in behat_form_field::set_value() there are two calls to the same hasAttribute() method, we can call it once and store the result in a var so we save 1 xpath query to the driver

            3) If we are not using a proper separator and we stick with the comma I prefer to wrap option/s in single quotes if they contain a comma rather than a new step but we will need to comment about it in the step description and I don't know what will be more confusing for a test writer:

            • If you are filling a multiselect separate the options in $$$
              or
            • If the option you provide contains a comma wrap it in quotes on the other hand, if you are filling a multiselect separate the options wrapping them in single quotes and separating them with commas

            I don't like non of the options and I agree that will make features ugly, but if in future we are going to need more separators, would be good to fix one and use the same for everything; separators would be very helpful when writting generic steps that delegate arguments to others. I thought on it many times and I ended up creating multiple step definitions. Otherwise we should provide the info about single quotes wrapping or escape commas in a smart way (also documenting it somewhere) Up to you.

            4) I've created MDL-43738, not sure about your an individual field “the FIELD should match VALUE” (apparently not supporting all fields) statement though, are you talking about selected items of a select box? I commented about getters in MDL-43713, if this is the case probably would be better to fix the generic step definition (maybe it has something to do with the commas) rather than creating new ones but please read the new issue

            6) Yes (sorry again) I understood it, I was suggesting that could be good to add a comment in the code so when we look at it we know why it is there as it is not obious and it seems easy to forget (at least I will forget) without the comment now seems unnecessarily complex

            8) A ok, I agree with you, it makes more sense like this, thanks for explaining

            Show
            David Monllaó added a comment - 2) Sorry, I didn't explain it properly, in behat_form_field::set_value() there are two calls to the same hasAttribute() method, we can call it once and store the result in a var so we save 1 xpath query to the driver 3) If we are not using a proper separator and we stick with the comma I prefer to wrap option/s in single quotes if they contain a comma rather than a new step but we will need to comment about it in the step description and I don't know what will be more confusing for a test writer: If you are filling a multiselect separate the options in $$$ or If the option you provide contains a comma wrap it in quotes on the other hand, if you are filling a multiselect separate the options wrapping them in single quotes and separating them with commas I don't like non of the options and I agree that will make features ugly, but if in future we are going to need more separators, would be good to fix one and use the same for everything; separators would be very helpful when writting generic steps that delegate arguments to others. I thought on it many times and I ended up creating multiple step definitions. Otherwise we should provide the info about single quotes wrapping or escape commas in a smart way (also documenting it somewhere) Up to you. 4) I've created MDL-43738 , not sure about your an individual field “the FIELD should match VALUE” (apparently not supporting all fields) statement though, are you talking about selected items of a select box? I commented about getters in MDL-43713 , if this is the case probably would be better to fix the generic step definition (maybe it has something to do with the commas) rather than creating new ones but please read the new issue 6) Yes (sorry again) I understood it, I was suggesting that could be good to add a comment in the code so when we look at it we know why it is there as it is not obious and it seems easy to forget (at least I will forget) without the comment now seems unnecessarily complex 8) A ok, I agree with you, it makes more sense like this, thanks for explaining
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Quick re-thoughs:

            1) tests: nothing new. will add a few, bot the the commons tests and to the wiki ones.

            2) hasAttribute(): ah! now it's clear for me, agree. will fix.

            3) aha, escaping... that's the key... I'll try to make it work ok by using \, for literal commas not being separators. let's see how it goes.

            4) I pretty much agree with the new MDL-43738. The main point there IMO, apart from putting all the getters and setters to work ok... is to decide which "individual" ones will be the future ones (deprecating the others). The generic one, or the specialised ones? In theory, to keep the language simpler and easier, I'd say that the generic one is a better alternative. And yes, we also need a multiple-checker to be paired with the "i fill the form". +100.

            And yes, I'm sure that current generic checker (the FIELD should match VALUE) is not working for all fields, because some get_value() implementations are using a straight getText(), like in multiple select boxes. Same applies to the generic setter (fill FIELD with VALUE) that is calling to the low level setValue() instead of the specialized ones. Let's talk about them in the new issue.

            5) nothing new.

            6) phpdocs/comments: aha will amend all the modified methods to clearly specify the changes, formats and what's going.

            7) more phpdocs/comments: same than before. Will complete missing explanations.

            8) ok, we leave the specialized new methods that way for now. no matter what's decided about them later in MDL-43738

            9) the explode in a loop, will fix.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Quick re-thoughs: 1) tests: nothing new. will add a few, bot the the commons tests and to the wiki ones. 2) hasAttribute(): ah! now it's clear for me, agree. will fix. 3) aha, escaping... that's the key... I'll try to make it work ok by using \, for literal commas not being separators. let's see how it goes. 4) I pretty much agree with the new MDL-43738 . The main point there IMO, apart from putting all the getters and setters to work ok... is to decide which "individual" ones will be the future ones (deprecating the others). The generic one, or the specialised ones? In theory, to keep the language simpler and easier, I'd say that the generic one is a better alternative. And yes, we also need a multiple-checker to be paired with the "i fill the form". +100. And yes, I'm sure that current generic checker (the FIELD should match VALUE) is not working for all fields, because some get_value() implementations are using a straight getText(), like in multiple select boxes. Same applies to the generic setter (fill FIELD with VALUE) that is calling to the low level setValue() instead of the specialized ones. Let's talk about them in the new issue. 5) nothing new. 6) phpdocs/comments: aha will amend all the modified methods to clearly specify the changes, formats and what's going. 7) more phpdocs/comments: same than before. Will complete missing explanations. 8) ok, we leave the specialized new methods that way for now. no matter what's decided about them later in MDL-43738 9) the explode in a loop, will fix. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Wow, working with get_value() for select elements, just discovered that the implementation, doing:

            $selectedoptions = $this->field->findAll('xpath', '//option[@selected="selected"]');
            

            Is really imperfect, because it only works if you save the form and edit it again. So, something like this:

            And I fill the moodle form with:
              | tags[officialtags][] | OT2, OT4 |
            And the "tags[officialtags][]" select box should contain "OT2, OT4" selected
            

            Does not work at all!

            Instead using Mink's getValue() seems to work like a charm. In all situations, both for saved and re-edited and not saved forms. I've added tests about that in the attached feature.

            Edited: I've reviewed all the other behat_form_xxxx types and it seems that selects were the only ones not using getValue() but the xpath search.

            Edited2: The get_value() alternative is working perfectly under Goutte, PhantomJS, Firefox, Chrome and Safari (with this I've detected a problem - in set_value() - caused by those extra click() leading to the select box getting incorrect values selected. If I comment out the extra click() calls, everything runs smoothly.

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Wow, working with get_value() for select elements, just discovered that the implementation, doing: $selectedoptions = $this->field->findAll('xpath', '//option[@selected="selected"]'); Is really imperfect, because it only works if you save the form and edit it again. So, something like this: And I fill the moodle form with: | tags[officialtags][] | OT2, OT4 | And the "tags[officialtags][]" select box should contain "OT2, OT4" selected Does not work at all! Instead using Mink's getValue() seems to work like a charm. In all situations, both for saved and re-edited and not saved forms. I've added tests about that in the attached feature. Edited: I've reviewed all the other behat_form_xxxx types and it seems that selects were the only ones not using getValue() but the xpath search. Edited2: The get_value() alternative is working perfectly under Goutte, PhantomJS, Firefox, Chrome and Safari (with this I've detected a problem - in set_value() - caused by those extra click() leading to the select box getting incorrect values selected. If I comment out the extra click() calls, everything runs smoothly.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Ok, I've added a new patch, to comment about (using original numbering):

            1a) tests: Finally I've not added new tests to admin/tool/behat/tests/behat/basic_actions.feature. Passing all existing + wiki ones (MDL-43673) should be enough to guarantee multi-selects are working as expected.
            1b) the attached feature file covers the multi-selects extensively, and are passing for all browsers but safari (because of those extra >click() killing the ongoing selection).

            2) The changes only apply to select boxes allowing multiple values. So all the old code should continue working the same. Now the $multiple var is calculated in all methods avoiding double calls.

            3) Ok, I've got it working keeping the commas as separators (as said, they were the best/more natural separator) and, if there is a comma in the text AND the field is a multi-select, then it can be escaped with backslash. Again, note that non-multiple selects do not need any escaping, they continue working as before. Only multi-selects having commas in text need to be escaped.

            4) Nothing new about MDL-43738. Yes we must decide about how to support the table/filed/specialized elements and go for that. Just a side note, I've added a comment to the_field_should_match_value() to clarify it does not support multi-selects and needs rework as part of MDL-43738. link

            5) 6) 7) I've added comments to all the params in the modified & created methods/steps:

            • the_select_box_should_contain() link
            • the_select_box_should_not_contain() link
            • the_select_box_should_contain_selected() link
            • the_select_box_should_contain_not_selected() link

            Again, note that they only change behavior/escaping/... if the field is a multi-select. No change for singles expected.

            8) Nothing new, we agreed they were the less worst alternatives for now.

            9) Fixed.

            10) I've changed the way behat_form_select->get_value() calculates selected values. Before the change it was looking for selected options and it only worked if the form has been saved and reloaded. With the change, selected items are immediately available to the API, without needing to save the form. That puts select boxes functionality on par with other element types.

            11) While working on 10) it was discovered that safari does not allow multiple set_value() to be performed without saving. It was discovered that the cause were those extra ->click() calls that are done, leading to wring options to be selected. As far as the use case (call multiple times to set_value() without saving the form is not a case used in core, it's not a big problem, but, for sure, the attached feature file reproduces it constantly. For your consideration if all those clicks require an issue to review them. Once commented out, Safari passes the feature without problems. In any case, please, take a look to the new method: link

            And that's all. I only have updated the master branch, after a peer-review round will backport it to 26_STABLE.

            Note I've performed these tests:

            • Attached -feature against goutte, firefox, chrome, safari (only 1 failure in Safari as commented above, pending to decide if requires an issue as far as it's not a "usual" case at all).
            • Complete run against firefox, chrome, phantomjs... running right now, but I think I've killed all the problems found in the previous runs. Will report once finished.

            Happy to get your wise thoughts about it...ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Ok, I've added a new patch, to comment about (using original numbering): 1a) tests: Finally I've not added new tests to admin/tool/behat/tests/behat/basic_actions.feature. Passing all existing + wiki ones ( MDL-43673 ) should be enough to guarantee multi-selects are working as expected. 1b) the attached feature file covers the multi-selects extensively, and are passing for all browsers but safari (because of those extra >click() killing the ongoing selection). 2) The changes only apply to select boxes allowing multiple values. So all the old code should continue working the same. Now the $multiple var is calculated in all methods avoiding double calls. 3) Ok, I've got it working keeping the commas as separators (as said, they were the best/more natural separator) and, if there is a comma in the text AND the field is a multi-select, then it can be escaped with backslash. Again, note that non-multiple selects do not need any escaping, they continue working as before. Only multi-selects having commas in text need to be escaped. 4) Nothing new about MDL-43738 . Yes we must decide about how to support the table/filed/specialized elements and go for that. Just a side note, I've added a comment to the_field_should_match_value() to clarify it does not support multi-selects and needs rework as part of MDL-43738 . link 5) 6) 7) I've added comments to all the params in the modified & created methods/steps: the_select_box_should_contain() link the_select_box_should_not_contain() link the_select_box_should_contain_selected() link the_select_box_should_contain_not_selected() link Again, note that they only change behavior/escaping/... if the field is a multi-select. No change for singles expected. 8) Nothing new, we agreed they were the less worst alternatives for now. 9) Fixed. 10) I've changed the way behat_form_select->get_value() calculates selected values. Before the change it was looking for selected options and it only worked if the form has been saved and reloaded. With the change, selected items are immediately available to the API, without needing to save the form. That puts select boxes functionality on par with other element types. 11) While working on 10) it was discovered that safari does not allow multiple set_value() to be performed without saving. It was discovered that the cause were those extra ->click() calls that are done, leading to wring options to be selected. As far as the use case (call multiple times to set_value() without saving the form is not a case used in core, it's not a big problem, but, for sure, the attached feature file reproduces it constantly. For your consideration if all those clicks require an issue to review them. Once commented out, Safari passes the feature without problems. In any case, please, take a look to the new method: link And that's all. I only have updated the master branch, after a peer-review round will backport it to 26_STABLE. Note I've performed these tests: Attached -feature against goutte, firefox, chrome, safari (only 1 failure in Safari as commented above, pending to decide if requires an issue as far as it's not a "usual" case at all). Complete run against firefox, chrome, phantomjs... running right now, but I think I've killed all the problems found in the previous runs. Will report once finished. Happy to get your wise thoughts about it...ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            (let's start a new numbering A...Z for any new comment)

            Show
            Eloy Lafuente (stronk7) added a comment - (let's start a new numbering A...Z for any new comment)
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Great, just detected that, with the new get_value() method, Goutte is failing, seems it does not keep the "selected" attribute updated. In the other side, it works with low level getValue(), but that cannot be used by selenium because it breaks with commas in the values. So seems I need some $this->running_javascript() conditional there to handle both cases. And surely also the 2 problems should be reported.

            Show
            Eloy Lafuente (stronk7) added a comment - Great, just detected that, with the new get_value() method, Goutte is failing, seems it does not keep the "selected" attribute updated. In the other side, it works with low level getValue(), but that cannot be used by selenium because it breaks with commas in the values. So seems I need some $this->running_javascript() conditional there to handle both cases. And surely also the 2 problems should be reported.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Done, here it's the last get_value() version, working with both selenium and goutte.

            https://github.com/stronk7/moodle/commit/cf57f3489184e0370df01e27a0e7f9ea93c7b091#diff-73977cc9bf283820a3c7d0b6759b149fL157

            • Selenium uses the "selected" attribute to look for selected values (because getValue() is broken for values containing commas).
            • Goutte uses getValue() to look for selected values (because the "selected" attribute is not refreshed with selections).

            Now it's back passing in all the browsers (but the Safari problem commented). And the whole run has ended here ok for firefox. So no regressions in existing single cases are expected.

            Niao!

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Done, here it's the last get_value() version, working with both selenium and goutte. https://github.com/stronk7/moodle/commit/cf57f3489184e0370df01e27a0e7f9ea93c7b091#diff-73977cc9bf283820a3c7d0b6759b149fL157 Selenium uses the "selected" attribute to look for selected values (because getValue() is broken for values containing commas). Goutte uses getValue() to look for selected values (because the "selected" attribute is not refreshed with selections). Now it's back passing in all the browsers (but the Safari problem commented). And the whole run has ended here ok for firefox. So no regressions in existing single cases are expected. Niao!
            Hide
            David Monllaó added a comment -

            Hi, I see that you are having fun I see no more issues, all good, I've run @core_group in phantomjs and chrome and all ok

            A) -> 5) 6) 7) When a test writer is listing the steps definitions they can only see the step description, which is the first line of the comment block, if you explain how to use them in the @param they will not know about how to use it until they read the step definition.

            Show
            David Monllaó added a comment - Hi, I see that you are having fun I see no more issues, all good, I've run @core_group in phantomjs and chrome and all ok A) -> 5) 6) 7) When a test writer is listing the steps definitions they can only see the step description, which is the first line of the comment block, if you explain how to use them in the @param they will not know about how to use it until they read the step definition.
            Hide
            CiBoT added a comment -

            Results for MDL-43713

            • Remote repository: git://github.com/stronk7/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43713 Remote repository: git://github.com/stronk7/moodle.git Remote branch MDL-43713 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/679 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/679/artifact/work/smurf.html
            Hide
            David Monllaó added a comment -

            Hi Eloy, talking about MDL-43738 (getters) but commenting here as it is related with The "ASD" select box should (not)? contain "SADASD" selected, I've been thinking about it and IMO the best solution is a behat_form_field::matches($expectedvalue) which can be overwritten by the children classes, so we can have a single and generic the_field_should_match_value() and delegate all the work to the field guesser (behat_form_field::guess_type()) and all the behat_form_field family; we would not need things like:

            • The "ASDASD" checkbox should be checked
            • The "ASDASD" checkbox should not be checked
            • The "ASD" select box should contain "SADASD" selected
            • The "ASD" select box should not contain "SADASD" selected

            I will work on top of MDL-43713 moving The "SELECT" select box should (not)? contain "option1" selected to behat_field_select::matches(), it will change a bit because, for example to do:

            The "SELECT" select box should (not)? contain "option1" selected
            The "SELECT" select box should (not)? contain "option2" selected
            

            We should do:

            The "SELECT" field should match "option1,option2"
            

            Which is different but IMO would be better to keep a single way to match values.

            We could mark both issues as blocking each other to join the testing efforts after being integrated, it would delay a bit this chain of issues, so not sure if you think is worth; in both cases the whole suite should be executed against multiple browsers.

            Show
            David Monllaó added a comment - Hi Eloy, talking about MDL-43738 (getters) but commenting here as it is related with The "ASD" select box should (not)? contain "SADASD" selected , I've been thinking about it and IMO the best solution is a behat_form_field::matches($expectedvalue) which can be overwritten by the children classes, so we can have a single and generic the_field_should_match_value() and delegate all the work to the field guesser (behat_form_field::guess_type()) and all the behat_form_field family; we would not need things like: The "ASDASD" checkbox should be checked The "ASDASD" checkbox should not be checked The "ASD" select box should contain "SADASD" selected The "ASD" select box should not contain "SADASD" selected I will work on top of MDL-43713 moving The "SELECT" select box should (not)? contain "option1" selected to behat_field_select::matches(), it will change a bit because, for example to do: The "SELECT" select box should (not)? contain "option1" selected The "SELECT" select box should (not)? contain "option2" selected We should do: The "SELECT" field should match "option1,option2" Which is different but IMO would be better to keep a single way to match values. We could mark both issues as blocking each other to join the testing efforts after being integrated, it would delay a bit this chain of issues, so not sure if you think is worth; in both cases the whole suite should be executed against multiple browsers.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            As commented via chat, David, feel free to get ownership of this together with MDL-43738 and add stuff on any. Just say who blocks who and done. Surely you you can use this as start point for MDL-43738 and apply all the changes there.

            Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - As commented via chat, David, feel free to get ownership of this together with MDL-43738 and add stuff on any. Just say who blocks who and done. Surely you you can use this as start point for MDL-43738 and apply all the changes there. Thanks!
            Hide
            David Monllaó added a comment -

            Thanks Eloy, I've been working in MDL-43738 these last days and I'm near finish testing a global solution, I had doubts about how to backport it though, I will comment in MDL-43738. I would prefer to work on top of your patch in MDL-43738 and close this issue so we don't have duplicate reviews, testing instructions...

            Show
            David Monllaó added a comment - Thanks Eloy, I've been working in MDL-43738 these last days and I'm near finish testing a global solution, I had doubts about how to backport it though, I will comment in MDL-43738 . I would prefer to work on top of your patch in MDL-43738 and close this issue so we don't have duplicate reviews, testing instructions...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Perfect, closing this. Let's continue @ MDL-43738. Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Perfect, closing this. Let's continue @ MDL-43738 . Thanks!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: