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

Normalize behat fields getters and setters

    Details

    • Testing Instructions:
      Hide

      Test 1

      1. Check that behat passes in all branches (you can ask integrators for the CI server status)

      Test 2 (only master)

      1. From dirroot, php admin/tool/behat/cli/init.php
      2. Ensure you didn't set $CFG->behat_usedeprecated = true; in your config.php
      3. Write a new .feature file using one of the deprecated methods (https://github.com/dmonllao/moodle/commit/ff056b98d5f05543e5b39b2c71dd967e766a800e#diff-6)
      4. From dirroot type php admin/tool/behat/util.php --enable
      5. Run vendor/bin/behat --config /asd/asd/asd/as/behat/behat.yml /absolute/path/to/the/feature/file/you/created/file.feature
      6. You SHOULD see a failure stating that that step was deprecated and proposing you an alternative
      7. Set $CFG->behat_usedeprecated = true; in your config.php
      8. Run vendor/bin/behat --config /asd/asd/asd/as/behat/behat.yml /absolute/path/to/the/feature/file/you/created/file.feature
      9. It SHOULD pass
      Show
      Test 1 Check that behat passes in all branches (you can ask integrators for the CI server status) Test 2 (only master) From dirroot, php admin/tool/behat/cli/init.php Ensure you didn't set $CFG->behat_usedeprecated = true; in your config.php Write a new .feature file using one of the deprecated methods ( https://github.com/dmonllao/moodle/commit/ff056b98d5f05543e5b39b2c71dd967e766a800e#diff-6 ) From dirroot type php admin/tool/behat/util.php --enable Run vendor/bin/behat --config /asd/asd/asd/as/behat/behat.yml /absolute/path/to/the/feature/file/you/created/file.feature You SHOULD see a failure stating that that step was deprecated and proposing you an alternative Set $CFG->behat_usedeprecated = true; in your config.php Run vendor/bin/behat --config /asd/asd/asd/as/behat/behat.yml /absolute/path/to/the/feature/file/you/created/file.feature It SHOULD pass
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-43738_master

      Description

      From https://tracker.moodle.org/browse/MDL-43713?focusedCommentId=265152&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-265152 discussion.

      We should normalize all field values getters and setters and update .feature files if necessary, what it makes sense is to only use the behat_form_field**(set|get)_value() methods

      In general we have multiple choices:

      • Provide a generic getter and setter
      • Provide a generic getter and setter + specific getters and setters
      • Provide only specific-field getters and setters

      IMO the best option will be a generic one (adding to the multi-field getter and setter) but I'm not sure it will be always possible or will restrict us more than help us.

      • Setters
        • Multi-field: I fill the moodle form with: delegates to the form_fields, which is good so no need to do anything else
        • Individual field:
          • Texts:
            • I've run:

              find . -name *.feature | xargs grep " I fill " | grep -v "fill the moodle form" | grep -v "fill the form"
              find . -name behat_*.php | xargs grep " I fill " | grep -v "fill the moodle form" | grep -v "fill the form"
              

            • All the uses of I fill in "ASD" with "ASD" are filling text fields, so I think that we should change the step description to let the users know that it is only inteded to be used to fill text-based fields. If we switch it to (get|set)_value() the behaviour will be the same as will default to behat_form_field. Another option would be to have a I fill in "ASD" with "ASD" text only for text-based fields and a new generic I fill in "ASD" with "ASD" checkboxes or radios don't expect a second argument and may lead to confusion, but in I fill the moodle form with: we solve it passing 0 or 1 as checked/unchecked the generic one can have the same behaviour than the multi I fill the moodle form with: but then the generic one or the specific ones would be rendundant
          • Selects. Are already using behat_form_select
          • Checkboxes. Will sounds vage, I know but I remember to have trouble here, that's why both have a different implementation
          • Radios. Similar to checkboxes, but as we are not using them extensively it would not be a problem to move to behat_form_field
      • Getters
        • Multi-field: We need a multiple one, like I fill the moodle form with:. We can do the same we do in I fill the moodle form with: and pass 0 or 1 to checkboxes and radios, something like the following fields should match the following values: and a cool 2 columns TableNode below.
        • Individual field: Here I doubt whether we have a specialized checker for all of the field types
          • behat_forms::the_field_should_match_value() is the generic one which does it's job and is widely used
          • About specific ones
            • Checkboxes. There are a couple of step definitions (checked and unchecked) that are only used in the feature where we check that the step definitions works as expected, so we can perfectly get rid of it
            • Selects: MDL-43713 introduces new steps here

      IMO to normalize the situation we could:

      1. Seems easy to deprecate the checkboxes one and fix the generic one to work with all fields, here we may find the same problems we will face in #2 below
      2. After #1 we can estimate if it is better to have a single step definition for setters of multiple ones

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dmonllao David Monllaó added a comment -

            Commenting now that I've remembered, the checkbox thing I commented about in Checkboxes. Will sounds vage, I know but I remember to have trouble here, that's why both have a different implementation

            There are 2 different JS events that we want to trigger when clicking in a checkbox, onchange and onclick, at the moment when we "click" onclick is triggered and when we "check" onchange is triggered, we need to join and force both events to be triggered in behat_form_checkbox::set_value()

            Show
            dmonllao David Monllaó added a comment - Commenting now that I've remembered, the checkbox thing I commented about in Checkboxes. Will sounds vage, I know but I remember to have trouble here, that's why both have a different implementation There are 2 different JS events that we want to trigger when clicking in a checkbox, onchange and onclick, at the moment when we "click" onclick is triggered and when we "check" onchange is triggered, we need to join and force both events to be triggered in behat_form_checkbox::set_value()
            Hide
            dmonllao David Monllaó added a comment -

            Adding WIP pull branch in case @stronk7 is curious about it; the implementation is finished and being tested, but at the moment 0 failures with the most problematic tests, pending to run a whole suite.

            We need to refine the available steps, but the backend side is correct.

            Show
            dmonllao David Monllaó added a comment - Adding WIP pull branch in case @stronk7 is curious about it; the implementation is finished and being tested, but at the moment 0 failures with the most problematic tests, pending to run a whole suite. We need to refine the available steps, but the backend side is correct.
            Hide
            dmonllao David Monllaó added a comment -

            The patch is ready, and passing 100% (at least in my computer with chrome and FF)

            It is introducing these new steps:

            • the "FIELD" field should not match "VALUE" value
            • the following fields should match:

            Following the issue's description notes, what I've done (More info in the commit messages about other minor changes introduced here):

            1. Getters
              • I fill in "FIELD" with "VALUE" is working for all kind of fields using behat_form_field (and family)
                • Expects fields like checkboxes, where there is not a "value" to use 1 or "" as value
              • Removed the 2 steps introduced by Eloy and replaced them by checkings based on *the "FIELD" field should not match "VALUE" value, completed Eloy's attached feature with other fields checkings and added it to the suite to be tested continuously as we will probably introduce more change in those functions.
              • TODO: I would vote to remove "CHECKBOX" checkbox should (not)? be checked steps in master and keep them in stables, they are not used at all in core, probably 3rd parties does
            1. Setters
              • I've refactored all the setters to use behat_form_field (& family) rather than using API calls so now we have many steps doing the same in the backend
              • TODO: I would vote to deprecate them all and move to a single I fill in "FIELD" with "VALUE" where checkboxes would use 1 or "" like when using getters, the current steps that can be merged into a single one are:
                • I fill in "FIELD" with "VALUE" (my candidate to "survivor")
                • I select "OPTION" from "SELECT"
                • I check "CHECKBOX"
                • I uncheck "CHECKBOX"
                • I select "RADIO" radio button

            I attached the patch, please comment about it before I begin deprecating methods, about branches I would propose to:

            • In master
              • Include all API fixes
              • Introduce the new 2 steps
              • TODO: Refactor all .feature files setters to use behat_form::the_field_should_match_value() and behat_form::fill_field()
              • TODO: Deprecate (todo)
                • I select "OPTION" from "SELECT"
                • I check "CHECKBOX"
                • I uncheck "CHECKBOX"
                • I select "RADIO" radio button
            • In stables
              • Include all API fixes
              • Introduce the new 2 steps so Eloy can use them in the blocked wiki tests (and following new scenarios too)

            With this all that test writers needs to know is that I fill in "FIELD" with "VALUE" sets a value in a field, the "FIELD" field should match "VALUE" value checks the field value, and that any value different that "" will be considered a checked checkbox and "" will be considered an unchecked checkbox.

            Show
            dmonllao David Monllaó added a comment - The patch is ready, and passing 100% (at least in my computer with chrome and FF) It is introducing these new steps: the "FIELD" field should not match "VALUE" value the following fields should match: Following the issue's description notes, what I've done (More info in the commit messages about other minor changes introduced here): Getters I fill in "FIELD" with "VALUE" is working for all kind of fields using behat_form_field (and family) Expects fields like checkboxes, where there is not a "value" to use 1 or "" as value Removed the 2 steps introduced by Eloy and replaced them by checkings based on *the "FIELD" field should not match "VALUE" value, completed Eloy's attached feature with other fields checkings and added it to the suite to be tested continuously as we will probably introduce more change in those functions. TODO: I would vote to remove "CHECKBOX" checkbox should (not)? be checked steps in master and keep them in stables, they are not used at all in core, probably 3rd parties does Setters I've refactored all the setters to use behat_form_field (& family) rather than using API calls so now we have many steps doing the same in the backend TODO: I would vote to deprecate them all and move to a single I fill in "FIELD" with "VALUE" where checkboxes would use 1 or "" like when using getters, the current steps that can be merged into a single one are: I fill in "FIELD" with "VALUE" (my candidate to "survivor") I select "OPTION" from "SELECT" I check "CHECKBOX" I uncheck "CHECKBOX" I select "RADIO" radio button I attached the patch, please comment about it before I begin deprecating methods, about branches I would propose to: In master Include all API fixes Introduce the new 2 steps TODO: Refactor all .feature files setters to use behat_form::the_field_should_match_value() and behat_form::fill_field() TODO: Deprecate (todo) I select "OPTION" from "SELECT" I check "CHECKBOX" I uncheck "CHECKBOX" I select "RADIO" radio button In stables Include all API fixes Introduce the new 2 steps so Eloy can use them in the blocked wiki tests (and following new scenarios too) With this all that test writers needs to know is that I fill in "FIELD" with "VALUE" sets a value in a field, the "FIELD" field should match "VALUE" value checks the field value, and that any value different that "" will be considered a checked checkbox and "" will be considered an unchecked checkbox.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            As far as the logbot does not like my explanations just putting them here:

            I'd just use:
             
            Setters:
             
            I set the form <form> with <table>
            I set the field <field> to <value(s)>
             
            Getters:
             
            The form <form> should contain <table>
            The field <field> should contain <value(s)> 
             
            with they supporting all field types.
             
            And the, there is one more getter to ask for selected options, like:
             
            The field <field> should contain <values(s)> selected
             
            And done

            I agree it can look not pretty in English for all fields, perhaps there is some more natural way to say that. If not possible, surely we'll should keep the specialised alternatives (surely simple wrappers) working. But I'd try to find a good step to cover all them, keeping the vocabulary reduced.

            Just my opinion, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - As far as the logbot does not like my explanations just putting them here: I'd just use:   Setters:   I set the form <form> with <table> I set the field <field> to <value(s)>   Getters:   The form <form> should contain <table> The field <field> should contain <value(s)>   with they supporting all field types.   And the, there is one more getter to ask for selected options, like:   The field <field> should contain <values(s)> selected   And done I agree it can look not pretty in English for all fields, perhaps there is some more natural way to say that. If not possible, surely we'll should keep the specialised alternatives (surely simple wrappers) working. But I'd try to find a good step to cover all them, keeping the vocabulary reduced. Just my opinion, ciao
            Hide
            timhunt Tim Hunt added a comment -

            I think I agree with the principle of having as few commands as possible. Less for people to learn in order to read and write tests.

            However, I would like to make the English read as naturally as possible, so I suggest the wording

            I set the field <field> to <value(s)>
            The field <field> has value <value(s)>

            and, for checkboxes, can we make the names for the values 'checked' and 'unchecked' so that it reads

            I set the field good_english to checked
            The field good_english has value checked

            etc. I don't know if this is possible.

            Show
            timhunt Tim Hunt added a comment - I think I agree with the principle of having as few commands as possible. Less for people to learn in order to read and write tests. However, I would like to make the English read as naturally as possible, so I suggest the wording I set the field <field> to <value(s)> The field <field> has value <value(s)> and, for checkboxes, can we make the names for the values 'checked' and 'unchecked' so that it reads I set the field good_english to checked The field good_english has value checked etc. I don't know if this is possible.
            Hide
            dmonllao David Monllaó added a comment -

            Thanks for the feedback.

            Eloy, the field has value like Tim proposes sounds correct and including selects, it is true that the contains means that the field can contain something else. Would be a pity to need a separate step that will introduce confusion.

            Tim, I agree that for checkboxes (or to check radio options) there is no need for a second argument, but we need the group getters and setters, so people will have to know that 1, yes, check or whatever value checks a checkbox and '' unchecks, also having all the fields BUT not this one sounds strange, IMO would be better to join them all into 1 step and explain in the step description or in the MDocs page what do we expect from users if they want to use them to set checkboxes.

            I'm happy with the I set the field "" to "" and the field "" has value "" but I'm not sure they express that the value "matches" and it includes all the field types; would you add any change if they are including checkboxes? I mean, thinking on multi-selects, the field "groups" has value "Student1" sounds like Student1 is a selected option, but does not imply that is the only one that is selected, something like the field "groups" matches value "Student1" means that it matches, with multiple selected options would be something like the field "groups" matches value "Student1, Student2".

            The more I write here more I see that it makes sense and is a long term solution to use 4 single steps (1 individual setter, 1 group setter, 1 individual getter and 1 group getter) and document in the MDocs page how to set and match values to each field, all the framework is ready to deal with all sort of form elements (http://docs.moodle.org/dev/Form_API#Form_elements) even the advanced ones, we just need to implement the class for it and define set_value(), get_value(), matches() and a format inside the "VALUE" argument to fill it, so a I set the field "ASD" to "ASD" will work with all, including the ones without arguments, the ones with a single argument, addGroup() elements and data_selectors & complex stuff like this; would be impossible to do it in a centralized way without proper documentation because users can not "guess" and the step description block is not enough for all the setters & getters we will define. We could do things like:

            • I set the field "Date available" to "01/03/2012" (which would auto-enable it if it is optional)
            • I set the field "Force language" to "yes" (or to 1 or to check)
            • The field "During the attempt" has value "The attempt, Whether correct"

            The alternative (which in my opinion is a lot worst) would be to have 1 different step for each field type...

            I will update the patch with a proposal and return here

            Show
            dmonllao David Monllaó added a comment - Thanks for the feedback. Eloy, the field has value like Tim proposes sounds correct and including selects, it is true that the contains means that the field can contain something else. Would be a pity to need a separate step that will introduce confusion. Tim, I agree that for checkboxes (or to check radio options) there is no need for a second argument, but we need the group getters and setters, so people will have to know that 1 , yes , check or whatever value checks a checkbox and '' unchecks, also having all the fields BUT not this one sounds strange, IMO would be better to join them all into 1 step and explain in the step description or in the MDocs page what do we expect from users if they want to use them to set checkboxes. I'm happy with the I set the field "" to "" and the field "" has value "" but I'm not sure they express that the value "matches" and it includes all the field types; would you add any change if they are including checkboxes? I mean, thinking on multi-selects, the field "groups" has value "Student1" sounds like Student1 is a selected option, but does not imply that is the only one that is selected, something like the field "groups" matches value "Student1" means that it matches, with multiple selected options would be something like the field "groups" matches value "Student1, Student2" . The more I write here more I see that it makes sense and is a long term solution to use 4 single steps (1 individual setter, 1 group setter, 1 individual getter and 1 group getter) and document in the MDocs page how to set and match values to each field, all the framework is ready to deal with all sort of form elements ( http://docs.moodle.org/dev/Form_API#Form_elements ) even the advanced ones, we just need to implement the class for it and define set_value(), get_value(), matches() and a format inside the "VALUE" argument to fill it, so a I set the field "ASD" to "ASD" will work with all, including the ones without arguments, the ones with a single argument, addGroup() elements and data_selectors & complex stuff like this; would be impossible to do it in a centralized way without proper documentation because users can not "guess" and the step description block is not enough for all the setters & getters we will define. We could do things like: I set the field "Date available" to "01/03/2012" (which would auto-enable it if it is optional) I set the field "Force language" to "yes" (or to 1 or to check) The field "During the attempt" has value "The attempt, Whether correct" The alternative (which in my opinion is a lot worst) would be to have 1 different step for each field type... I will update the patch with a proposal and return here
            Hide
            dmonllao David Monllaó added a comment - - edited

            In total we need 6 steps, adding candidates, please refine with your English skills:

            I set the field "FIELD" to "VALUE"
            

            I set the following fields to these values:
              | FIELD1 | VALUE1 |
              | FIELD2 | VALUE2 |
            

            The field "FIELD" matches value "VALUE"
            

            the following fields match these values:
              | FIELD1 | VALUE1 |
              | FIELD2 | VALUE2 |
            

            The field "FIELD" does not match value "VALUE"
            

            the following fields do not match these values:
              | FIELD1 | VALUE1 |
              | FIELD2 | VALUE2 |
            

            I'm referring to "field" as they can be used with any field not only moodle forms.

            Show
            dmonllao David Monllaó added a comment - - edited In total we need 6 steps, adding candidates, please refine with your English skills: I set the field "FIELD" to "VALUE" I set the following fields to these values: | FIELD1 | VALUE1 | | FIELD2 | VALUE2 | The field "FIELD" matches value "VALUE" the following fields match these values: | FIELD1 | VALUE1 | | FIELD2 | VALUE2 | The field "FIELD" does not match value "VALUE" the following fields do not match these values: | FIELD1 | VALUE1 | | FIELD2 | VALUE2 | I'm referring to "field" as they can be used with any field not only moodle forms.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            Hi David,

            These seem good to me. I can't think a better way of wording it, so +1.

            (I just don't think there is a good way to write a list of things (2*x) any better than the way you've done it).

            Show
            poltawski Dan Poltawski added a comment - - edited Hi David, These seem good to me. I can't think a better way of wording it, so +1. (I just don't think there is a good way to write a list of things (2*x) any better than the way you've done it).
            Hide
            dmonllao David Monllaó added a comment -

            As commented I've fixed and normalized the fields and added the new steps in stables and the same in master + adding new steps according to the former comment + deprecated old methods + update .features and steps definitions calling them.

            dev_docs_required label was already added to the issue; to help users to fill the correct values using this generic step definitions I've added an info box (when users lists the step definitions, in admin/tool/behat/index.php) to FIELD_VALUE_STRING fields, so all steps expecting a field value will use this name (field_value_string) which will be replaced by the link to the MDocs page.

            The info in the MDocs page would be added to http://docs.moodle.org/dev/Acceptance_testing#Providing_values_to_steps, it could be (wiki format to copy & paste later):

            • '''A field value'''; There are many different field types, if an argument requires a field value the expected value will depend on the field type:
              • Text-based fields: It expects the text. This includes textareas, input type text, input type password...
              • Checkbox: It expects 1 to check and for checked and "" to uncheck or for unchecked
              • Select: It expects the option text or the option value. In case you interact with a multi-select you should specify the options separating them with commas. For example: '''option1, option2, option3'''
              • Radio: The text of the radio option

            In case is useful for anyone, for the deprecation I've used this:

            ##
            # Replaces all ocurrencies of $1 by $2. Skips hidden files.
            #
            # $4 exists to ensure we will not run the command
            # by mistake.
            #
            # $1 => The origin string
            # $2 => The replacement string
            # $3 => -name option value, leave empty ("") if not applicable
            # $4 => Needs to be set to execute the substitution, for safety.
            ##
            function replace_take_care_with_me {
             
                # Correct number of arguments.
                if [ -z "$2" ]; then
                    echo "Usage:
              replace_take_care_with_me origin replacement [name-option-pattern] [1]
             
              Examples:
                replace_take_care_with_me origin replacement *.php # To see the expected changes
                replace_take_care_with_me origin replacement *.php 1 # To execute the substitution
                "
                return
                fi
             
                # Not compulsory.
                local namestr=""
                if [ ! -z $3 ] and [ $3 != "1" ]; then
                    namestr="-name $3"
                fi
             
                # Just list the affected files.
                if [ -z "$4" ]; then
                    find . \( ! -regex '.*/\..*' \) -type f $namestr -print0 | xargs -0 grep "$1"
                else
                    replacement="s/$1/$2/g"
                    find . \( ! -regex '.*/\..*' \) -type f $namestr -print0 | xargs -0 sed -i -e "$replacement"
                fi
             
            }
             
            # Calling (considering behat_deprecated.php should remain the same)
            # replace_take_care_with_me "I fill in\(.*\)with" "I set the field\1to" "" 1
            # replace_take_care_with_me "I fill the moodle form with" "I set the following fields to these values" "" 1
            # replace_take_care_with_me "I select \(.*\) from \(.*\)\"" "I set the field \2\" to \1" "" 1
            # replace_take_care_with_me "I select \(.*\) radio button" "I set the field \1 to \"1\"" "" 1
            # replace_take_care_with_me "I check \(.*\)\"" "I set the field \1\" to \"1\"" "" 1
            # replace_take_care_with_me "I uncheck \(.*\)\"" "I set the field \1\" to \"\"" "" 1
            # replace_take_care_with_me "the \(.*\) field should match \(.*\) value" "the field \1 matches value \2" "" 1
            # replace_take_care_with_me "the \(.*\) checkbox should be checked" "the field \1 matches value \"1\"" *.feature 1
            

            Show
            dmonllao David Monllaó added a comment - As commented I've fixed and normalized the fields and added the new steps in stables and the same in master + adding new steps according to the former comment + deprecated old methods + update .features and steps definitions calling them. dev_docs_required label was already added to the issue; to help users to fill the correct values using this generic step definitions I've added an info box (when users lists the step definitions, in admin/tool/behat/index.php) to FIELD_VALUE_STRING fields, so all steps expecting a field value will use this name (field_value_string) which will be replaced by the link to the MDocs page. The info in the MDocs page would be added to http://docs.moodle.org/dev/Acceptance_testing#Providing_values_to_steps , it could be (wiki format to copy & paste later): '''A field value'''; There are many different field types, if an argument requires a field value the expected value will depend on the field type: Text-based fields: It expects the text. This includes textareas, input type text, input type password... Checkbox: It expects 1 to check and for checked and "" to uncheck or for unchecked Select: It expects the option text or the option value. In case you interact with a multi-select you should specify the options separating them with commas. For example: '''option1, option2, option3''' Radio: The text of the radio option In case is useful for anyone, for the deprecation I've used this: ## # Replaces all ocurrencies of $1 by $2. Skips hidden files. # # $4 exists to ensure we will not run the command # by mistake. # # $1 => The origin string # $2 => The replacement string # $3 => -name option value, leave empty ("") if not applicable # $4 => Needs to be set to execute the substitution, for safety. ## function replace_take_care_with_me {   # Correct number of arguments. if [ -z "$2" ]; then echo "Usage: replace_take_care_with_me origin replacement [name-option-pattern] [1]   Examples: replace_take_care_with_me origin replacement *.php # To see the expected changes replace_take_care_with_me origin replacement *.php 1 # To execute the substitution " return fi   # Not compulsory. local namestr="" if [ ! -z $3 ] and [ $3 != "1" ]; then namestr="-name $3" fi   # Just list the affected files. if [ -z "$4" ]; then find . \( ! -regex '.*/\..*' \) -type f $namestr -print0 | xargs -0 grep "$1" else replacement="s/$1/$2/g" find . \( ! -regex '.*/\..*' \) -type f $namestr -print0 | xargs -0 sed -i -e "$replacement" fi   }   # Calling (considering behat_deprecated.php should remain the same) # replace_take_care_with_me "I fill in\(.*\)with" "I set the field\1to" "" 1 # replace_take_care_with_me "I fill the moodle form with" "I set the following fields to these values" "" 1 # replace_take_care_with_me "I select \(.*\) from \(.*\)\"" "I set the field \2\" to \1" "" 1 # replace_take_care_with_me "I select \(.*\) radio button" "I set the field \1 to \"1\"" "" 1 # replace_take_care_with_me "I check \(.*\)\"" "I set the field \1\" to \"1\"" "" 1 # replace_take_care_with_me "I uncheck \(.*\)\"" "I set the field \1\" to \"\"" "" 1 # replace_take_care_with_me "the \(.*\) field should match \(.*\) value" "the field \1 matches value \2" "" 1 # replace_take_care_with_me "the \(.*\) checkbox should be checked" "the field \1 matches value \"1\"" *.feature 1
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43738

            • Remote repository: git://github.com/dmonllao/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43738 Remote repository: git://github.com/dmonllao/moodle.git Remote branch MDL-43738 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/940 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/940/artifact/work/smurf.html Remote branch MDL-43738 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/941 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/941/artifact/work/smurf.html Remote branch MDL-43738 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/942 Error: The MDL-43738 _master branch at git://github.com/dmonllao/moodle.git does not apply clean to master
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi David,

            I like your 6 proposal too. Some comments/questions about them:

            a) Do all selectors support "id|name|label" ? So we can use any of them to determine the "FIELD".

            b) As far as we are talking about "matches"... do we accept regexp or at least wildcards in "VALUE" or always complete matching, aka, totally equal.

            c) When handling some types having a value and a text, say select lists or menus... do we support both the value and its text/label. Both in setters and getters? Or only values?

            d) I assume that, when we are talking about "VALUE" all the time, is because the methods, when applied to selects, menus... only will set/get the "SELECTED options" correct? How do we verify if any other element is in the select/menu? I mean, previously we had (to be deprecated/out now):

            (to verify if an option existed or no)
             
            the "ELEMENT" select box should contain "OPTIONS"
            the "ELEMENT" select box should not contain "OPTIONS"

            (to verify if an option was selected or no)
             
            the "ELEMENT" select box should contain "OPTIONS" selected
            the "ELEMENT" select box should contain "OPTIONS" not selected

            And it's not clear for me to which of those groups the new setters/getters are replacing (I think that to the 2nd group). In any case, how is the other group handled?

            Side note: I've not looked to code at all, sorry, cannot right now. Just sharing thoughts.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi David, I like your 6 proposal too. Some comments/questions about them: a) Do all selectors support "id|name|label" ? So we can use any of them to determine the "FIELD". b) As far as we are talking about "matches"... do we accept regexp or at least wildcards in "VALUE" or always complete matching, aka, totally equal. c) When handling some types having a value and a text, say select lists or menus... do we support both the value and its text/label. Both in setters and getters? Or only values? d) I assume that, when we are talking about "VALUE" all the time, is because the methods, when applied to selects, menus... only will set/get the "SELECTED options" correct? How do we verify if any other element is in the select/menu? I mean, previously we had (to be deprecated/out now): (to verify if an option existed or no)   the "ELEMENT" select box should contain "OPTIONS" the "ELEMENT" select box should not contain "OPTIONS" (to verify if an option was selected or no)   the "ELEMENT" select box should contain "OPTIONS" selected the "ELEMENT" select box should contain "OPTIONS" not selected And it's not clear for me to which of those groups the new setters/getters are replacing (I think that to the 2nd group). In any case, how is the other group handled? Side note: I've not looked to code at all, sorry, cannot right now. Just sharing thoughts.
            Hide
            dmonllao David Monllaó added a comment - - edited

            Thanks for the comments Eloy:

            a) Yes, https://github.com/Behat/Mink/blob/v1.5.0/src/Behat/Mink/Selector/NamedSelector.php#L25
            b) No, we don't accept wildcards; we can have enough with this set of values because the test writer controls all the contents of the site and the previous steps so he/she must know when something will match or will not match
            c) Yes, but it depends on the field type, we delegated all that process to lib/behat/form_fields/behat_form_field family so each field type matches() method will match against whatever the field considers; in case of selects it works with both options values and options text (https://github.com/dmonllao/moodle/blob/MDL-43738_master/lib/behat/form_field/behat_form_select.php#L240)
            d) The first two steps are still available as they states that a select box "contains" an option and we don't have other steps to check it; but I've removed the last 2 steps (the select box should contain "ASD" (not)? selected) in favour of the generic ones (like I've done with all other specific steps definitions) as the test writer have full control over the site contents and the executed steps results, and he/she should be able to check a select box contents providing all the select box selected options.

            I've externalized MDL-43951 from this patch and updated the pull branches to point the base to MDL-43951_*. Keeping it under waiting for peer review.

            Show
            dmonllao David Monllaó added a comment - - edited Thanks for the comments Eloy: a) Yes, https://github.com/Behat/Mink/blob/v1.5.0/src/Behat/Mink/Selector/NamedSelector.php#L25 b) No, we don't accept wildcards; we can have enough with this set of values because the test writer controls all the contents of the site and the previous steps so he/she must know when something will match or will not match c) Yes, but it depends on the field type, we delegated all that process to lib/behat/form_fields/behat_form_field family so each field type matches() method will match against whatever the field considers; in case of selects it works with both options values and options text ( https://github.com/dmonllao/moodle/blob/MDL-43738_master/lib/behat/form_field/behat_form_select.php#L240 ) d) The first two steps are still available as they states that a select box "contains" an option and we don't have other steps to check it; but I've removed the last 2 steps (the select box should contain "ASD" (not)? selected) in favour of the generic ones (like I've done with all other specific steps definitions) as the test writer have full control over the site contents and the executed steps results, and he/she should be able to check a select box contents providing all the select box selected options. I've externalized MDL-43951 from this patch and updated the pull branches to point the base to MDL-43951 _*. Keeping it under waiting for peer review.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43738 Remote repository: git://github.com/dmonllao/moodle.git Remote branch MDL-43738 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1058 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1058/artifact/work/smurf.html Remote branch MDL-43738 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1059 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1059/artifact/work/smurf.html Remote branch MDL-43738 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1060 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1060/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43738 Remote repository: git://github.com/dmonllao/moodle.git Remote branch MDL-43738 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1061 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1061/artifact/work/smurf.html Remote branch MDL-43738 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1062 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1062/artifact/work/smurf.html Remote branch MDL-43738 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1063 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1063/artifact/work/smurf.html
            Hide
            dmonllao David Monllaó added a comment -

            I wonder if would be "fair" to backport all new steps to 2.5 and 2.6, will make backport of new features much easier and faster, otherwise we will probably face many failures due to blindly backport .feature files. Any opinion?

            Show
            dmonllao David Monllaó added a comment - I wonder if would be "fair" to backport all new steps to 2.5 and 2.6, will make backport of new features much easier and faster, otherwise we will probably face many failures due to blindly backport .feature files. Any opinion?
            Hide
            timhunt Tim Hunt added a comment -

            +1 from me to backport all behat improvements. They won't break live sites, and they will make it possible to use behat in Moodle 2.6 (which the OU will be using for another 12 months) in a way that will not break later.

            Show
            timhunt Tim Hunt added a comment - +1 from me to backport all behat improvements. They won't break live sites, and they will make it possible to use behat in Moodle 2.6 (which the OU will be using for another 12 months) in a way that will not break later.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            As far as they don't break anything and are 100% additive, np here either.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - As far as they don't break anything and are 100% additive, np here either.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43738 Remote repository: git://github.com/dmonllao/moodle.git Remote branch MDL-43738 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1256 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1256/artifact/work/smurf.html Remote branch MDL-43738 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1257 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1257/artifact/work/smurf.html Remote branch MDL-43738 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1258 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1258/artifact/work/smurf.html
            Hide
            dmonllao David Monllaó added a comment -

            I've backported the new step definitions to 2.5 and 2.6.

            About the prechecker results I doubt about changing all constants to uppercase, must we?

            Sending to peer review again

            Show
            dmonllao David Monllaó added a comment - I've backported the new step definitions to 2.5 and 2.6. About the prechecker results I doubt about changing all constants to uppercase, must we? Sending to peer review again
            Hide
            timhunt Tim Hunt added a comment -

            Yes. Of course consts must be in upper-case. That is part of the Moodle coding style.

            Show
            timhunt Tim Hunt added a comment - Yes. Of course consts must be in upper-case. That is part of the Moodle coding style.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43738 Remote repository: git://github.com/dmonllao/moodle.git Remote branch MDL-43738 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1281 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1281/artifact/work/smurf.html Remote branch MDL-43738 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1282 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1282/artifact/work/smurf.html Remote branch MDL-43738 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1283 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1283/artifact/work/smurf.html
            Hide
            dmonllao David Monllaó added a comment -

            Yes, thanks Tim, what I wanted to do is to avoid deprecating 3 constants in favour of the same ones but in uppercase. I've changed them for get_string calls, the values were hardcoded to avoid updating features and step definitions descriptions after language string changes but we took the general approach of updating steps after language string changes so makes sense to follow the same rule; I've removed the constants from master adding info about it's deprecation in admin/tool/behat/upgrade.txt.

            Pull branches updated

            Show
            dmonllao David Monllaó added a comment - Yes, thanks Tim, what I wanted to do is to avoid deprecating 3 constants in favour of the same ones but in uppercase. I've changed them for get_string calls, the values were hardcoded to avoid updating features and step definitions descriptions after language string changes but we took the general approach of updating steps after language string changes so makes sense to follow the same rule; I've removed the constants from master adding info about it's deprecation in admin/tool/behat/upgrade.txt. Pull branches updated
            Hide
            dmonllao David Monllaó added a comment -

            Hi Eloy, and chance that you can review this? This fixes includes changes in composer.json and it is blocking MDL-44286 which would be good to integrate as soon as possible, otherwise I will rebase this issue on top of MDL-44286

            Show
            dmonllao David Monllaó added a comment - Hi Eloy, and chance that you can review this? This fixes includes changes in composer.json and it is blocking MDL-44286 which would be good to integrate as soon as possible, otherwise I will rebase this issue on top of MDL-44286
            Hide
            dmonllao David Monllaó added a comment -

            Adding Rajesh as PR to send this to integration ASAP; I've referenced this issue's changes a few times over the last week, is slow and hard to rebase as we are deprecating basic step definitiong and every week new features using those steps lands, also it includes a change in composer.json which makes all other changes in composer.json blocked by this one (or an order switch like I've done in MDL-44286 which is manual and time consuming too)

            Show
            dmonllao David Monllaó added a comment - Adding Rajesh as PR to send this to integration ASAP; I've referenced this issue's changes a few times over the last week, is slow and hard to rebase as we are deprecating basic step definitiong and every week new features using those steps lands, also it includes a change in composer.json which makes all other changes in composer.json blocked by this one (or an order switch like I've done in MDL-44286 which is manual and time consuming too)
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for fixing this David,

            Patch looks great, and new wording is for sure better...

            I couldn't find much with coding, just few minor things you might want to consider:

            1. Use of httpswwwroot is not required for popup. I had a word with Petr about this as well and he agrees with it.
            2. Should https://github.com/dmonllao/moodle/compare/MDL-44286_master...MDL-43738_master#diff-d8b8fbbfaeaaff594bfb6bae96298a39R141 be removed (empty scenario)

            I ran behat selenium with you patch and it's failing with memory exhausted error. (I ran it twice and got same problem)

            PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 4265 bytes) in /home/rajesht/moodles/m/moodle/vendor/instaclick/php-webdriver/lib/WebDriver/Service/CurlService.php on line 83
            

            FYI: Above issue happens at rubric scenario (@gadingform_rubric). Seems to hang the process. I am still investigating the problem.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this David, Patch looks great, and new wording is for sure better... I couldn't find much with coding, just few minor things you might want to consider: Use of httpswwwroot is not required for popup. I had a word with Petr about this as well and he agrees with it. Should https://github.com/dmonllao/moodle/compare/MDL-44286_master...MDL-43738_master#diff-d8b8fbbfaeaaff594bfb6bae96298a39R141 be removed (empty scenario) I ran behat selenium with you patch and it's failing with memory exhausted error. (I ran it twice and got same problem) PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 4265 bytes) in /home/rajesht/moodles/m/moodle/vendor/instaclick/php-webdriver/lib/WebDriver/Service/CurlService.php on line 83 FYI: Above issue happens at rubric scenario (@gadingform_rubric). Seems to hang the process. I am still investigating the problem.
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Aha...
            In lib/behat/form_field/behat_form_field.php matches gets into recursion as form_editor doesn't have matches overridden and it calls itself.
            You need to check if $instance is type of behat_form_editor then use get_value comparison.

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Aha... In lib/behat/form_field/behat_form_field.php matches gets into recursion as form_editor doesn't have matches overridden and it calls itself. You need to check if $instance is type of behat_form_editor then use get_value comparison.
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Rajesh, good catch, I've refactored the field type guesser keeping backwards compatibility.

            About the first review points:
            1) Patch updated
            2) The empty scenarios runs the background section

            Show
            dmonllao David Monllaó added a comment - Thanks Rajesh, good catch, I've refactored the field type guesser keeping backwards compatibility. About the first review points: 1) Patch updated 2) The empty scenarios runs the background section
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43738

            • Remote repository: git://github.com/dmonllao/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43738 Remote repository: git://github.com/dmonllao/moodle.git Remote branch MDL-43738 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1799 Error: The MDL-43738 _25 branch at git://github.com/dmonllao/moodle.git exceeds the maximum number of commits ( 24 > 15) Remote branch MDL-43738 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1800 Error: The MDL-43738 _26 branch at git://github.com/dmonllao/moodle.git exceeds the maximum number of commits ( 27 > 15) Remote branch MDL-43738 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1801 Error: The MDL-43738 _master branch at git://github.com/dmonllao/moodle.git exceeds the maximum number of commits ( 62 > 15)
            Hide
            marina Marina Glancy added a comment - - edited

            comment deleted (sorry commented on wrong issue)

            Show
            marina Marina Glancy added a comment - - edited comment deleted (sorry commented on wrong issue)
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            marina Marina Glancy added a comment -

            I raised the priority because there are several other issues that can be affected by whether this one gets integrated or not

            Show
            marina Marina Glancy added a comment - I raised the priority because there are several other issues that can be affected by whether this one gets integrated or not
            Hide
            damyon Damyon Wiese added a comment -

            Thanks David / Eloy.

            I agree with the deprecation/backport changes here. Will let the ci server run the behat suites - the changes themselves make sense.

            Integrated to 25, 26 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks David / Eloy. I agree with the deprecation/backport changes here. Will let the ci server run the behat suites - the changes themselves make sense. Integrated to 25, 26 and master.
            Hide
            phalacee Jason Fowler added a comment -

            Test 2 passed. Waiting on iTeam to pass the issue after the CI server passes the behat tests.

            Show
            phalacee Jason Fowler added a comment - Test 2 passed. Waiting on iTeam to pass the issue after the CI server passes the behat tests.
            Hide
            dmonllao David Monllaó added a comment -

            Hi Jason, we were waiting for master to pass and it failed with a non-related failure (http://nightly01:8080/view/M3.%20Behat/job/B6.3.-%20Run%20all%20behat%20features%20in%20firefox-linux%20(master)/349/console) Rajesh just restarted the job with a fix included for it, feel free to pass it when you want to if you prefer wait for the next run to finish.

            Show
            dmonllao David Monllaó added a comment - Hi Jason, we were waiting for master to pass and it failed with a non-related failure ( http://nightly01:8080/view/M3.%20Behat/job/B6.3.-%20Run%20all%20behat%20features%20in%20firefox-linux%20(master)/349/console ) Rajesh just restarted the job with a fix included for it, feel free to pass it when you want to if you prefer wait for the next run to finish.
            Hide
            phalacee Jason Fowler added a comment -

            all passing thanks David

            Show
            phalacee Jason Fowler added a comment - all passing thanks David
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            For fun: http://www.youtube.com/watch?v=IGENkpaPkgw

            Many thanks for your hard work, this is now part of Moodle!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - For fun: http://www.youtube.com/watch?v=IGENkpaPkgw Many thanks for your hard work, this is now part of Moodle! Ciao
            Hide
            dmonllao David Monllaó added a comment -

            Removing dev_docs_required label as http://docs.moodle.org/dev/Acceptance_testing#Providing_values_to_steps has been updated

            Show
            dmonllao David Monllaó added a comment - Removing dev_docs_required label as http://docs.moodle.org/dev/Acceptance_testing#Providing_values_to_steps has been updated
            Hide
            marina Marina Glancy added a comment -

            Hi David Monllaó, this issue is closed as fixed but there is still a comment in code:

            $ git grep MDL-43738
            grade/grading/form/rubric/tests/behat/behat_gradingform_rubric.php:                // TODO MDL-43738: Change setValue() to use the generic set_value()
            

            it will be very appreciated in it can be removed. TIA

            Show
            marina Marina Glancy added a comment - Hi David Monllaó , this issue is closed as fixed but there is still a comment in code: $ git grep MDL-43738 grade/grading/form/rubric/tests/behat/behat_gradingform_rubric.php: // TODO MDL-43738: Change setValue() to use the generic set_value() it will be very appreciated in it can be removed. TIA
            Hide
            dmonllao David Monllaó added a comment -

            Hi, thanks Marina, probably Rajesh Taneja knows what is the current status of this. Can it be removed Rajesh?

            Show
            dmonllao David Monllaó added a comment - Hi, thanks Marina, probably Rajesh Taneja knows what is the current status of this. Can it be removed Rajesh?
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            David Monllaó and Marina Glancy, Yes it can be removed. Should there be a new issue for this ?

            Show
            rajeshtaneja Rajesh Taneja added a comment - David Monllaó and Marina Glancy , Yes it can be removed. Should there be a new issue for this ?

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Mar/14