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

introduce $form->mock_submit(); method to test form submission

    Details

      Description

      A mock_submit method should make the form behave as if a submit button has been pressed and get_data should then return all the values as passed into the form from set_data and setDefault etc. So that we could perform the following integration test for example :

      $questiondata = test_question_maker::get_question_data('multichoice', 'single');
      $expectedfromform = test_question_maker::get_question_form_data('multichoice', 'single');
      $form = new qtype_multichoice_edit_form(...);
      $form->set_data($questiondata);
      $form->mock_submit(); 
      $actualfromform = $form->get_data();
      $this->assertEquals($expectedfromform, $actualfromform);
      save_question($actualfromform);
      $actualquestiondata = question_load_questions(array($actualfromform->id));
      $this->assertEquals($questiondata, $actualquestiondata);
      

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Yes. Nice idea.

            Show
            salvetore Michael de Raadt added a comment - Yes. Nice idea.
            Hide
            jamiesensei Jamie Pratt added a comment -

            Can you take a look at this Tim and see if you think it is the right approach for mock_submit. It is a small patch and it depends on the fix for MDL-40190 which is also in the branch above.

            I will right some tests for question saving which will use this method see the linked MDL-40171 which I am working on. I think those tests will also be adequate tests for the new mock_submit method and the select elements thing.

            Ah! Actually maybe I should also right some tests using some form classes written just for the purposes of testing mock_submit and for testing the select issue. Particularly I think for the select issue I need to write some tests to check that existing forms will not be affected.

            So can you take a look at this patch Tim, have you a different suggestion for how mock_submit will work. In the mean time I will write some unit tests for this code and they might be done before you have had a chance to see this, I will comment here when the tests are done. I figure that I should probably submit this peer review request now though since I have filled out the form already and I do want you to look at the code if you have time.

            Thanks.

            Show
            jamiesensei Jamie Pratt added a comment - Can you take a look at this Tim and see if you think it is the right approach for mock_submit. It is a small patch and it depends on the fix for MDL-40190 which is also in the branch above. I will right some tests for question saving which will use this method see the linked MDL-40171 which I am working on. I think those tests will also be adequate tests for the new mock_submit method and the select elements thing. Ah! Actually maybe I should also right some tests using some form classes written just for the purposes of testing mock_submit and for testing the select issue. Particularly I think for the select issue I need to write some tests to check that existing forms will not be affected. So can you take a look at this patch Tim, have you a different suggestion for how mock_submit will work. In the mean time I will write some unit tests for this code and they might be done before you have had a chance to see this, I will comment here when the tests are done. I figure that I should probably submit this peer review request now though since I have filled out the form already and I do want you to look at the code if you have time. Thanks.
            Hide
            jamiesensei Jamie Pratt added a comment -

            Oops, I realise I am not sure whether I should have left the tracker to choose who to do the peer review. I guess that it will be helpful to have Tim look at this first and then we can assign someone else to peer review too if desirable.

            Show
            jamiesensei Jamie Pratt added a comment - Oops, I realise I am not sure whether I should have left the tracker to choose who to do the peer review. I guess that it will be helpful to have Tim look at this first and then we can assign someone else to peer review too if desirable.
            Hide
            timhunt Tim Hunt added a comment -

            An alternative implementation would be

            public function mock_submit($simulatedpostdata = array()) {
                // ...
            }

            but I don't know how easy that would be to implement.

            I think the best person to get feedback from on this issue would be Petr Skoda, if he has time to take a look.

            Apart from that, two other comments:

            1. "The difference is for elements which have no default ..." bit worries me.

            2. function mockSubmit(){ is missing public and a space between ) and {.

            Show
            timhunt Tim Hunt added a comment - An alternative implementation would be public function mock_submit($simulatedpostdata = array()) { // ... } but I don't know how easy that would be to implement. I think the best person to get feedback from on this issue would be Petr Skoda , if he has time to take a look. Apart from that, two other comments: 1. "The difference is for elements which have no default ..." bit worries me. 2. function mockSubmit(){ is missing public and a space between ) and {.
            Hide
            jamiesensei Jamie Pratt added a comment -

            I have taken a new approach to mock_submit as you suggested Tim.

            I have one passing test, testing the question editing forms here : https://tracker.moodle.org/browse/MDL-40171

            Would be interested in opinions on the direction I am going with this. I have added Petr as a watcher since I believe he is the best person to comment on this area.

            I will have some more tests done and maybe some more ideas about how mock_submit should work after the weekend.

            Show
            jamiesensei Jamie Pratt added a comment - I have taken a new approach to mock_submit as you suggested Tim. I have one passing test, testing the question editing forms here : https://tracker.moodle.org/browse/MDL-40171 Would be interested in opinions on the direction I am going with this. I have added Petr as a watcher since I believe he is the best person to comment on this area. I will have some more tests done and maybe some more ideas about how mock_submit should work after the weekend.
            Hide
            timhunt Tim Hunt added a comment -

            This looks good to me. +1. Nice that it is so simple.

            I think Petr is in Melbourne for the next few days, at the MoodleMoot, but hopefully he will get a chance to look at this before it is submitted for integration.

            Show
            timhunt Tim Hunt added a comment - This looks good to me. +1. Nice that it is so simple. I think Petr is in Melbourne for the next few days, at the MoodleMoot, but hopefully he will get a chance to look at this before it is submitted for integration.
            Hide
            skodak Petr Skoda added a comment -

            +1, btw I could not go to AU, I am still stuck here in CZ

            Show
            skodak Petr Skoda added a comment - +1, btw I could not go to AU, I am still stuck here in CZ
            Hide
            timhunt Tim Hunt added a comment -

            Oh. That sucks. Thanks for the review.

            Show
            timhunt Tim Hunt added a comment - Oh. That sucks. Thanks for the review.
            Hide
            jamiesensei Jamie Pratt added a comment -

            The first version of mock_submit is not going to work for many forms. Many forms access submitted data through optional_param or similar, including all forms that use the repeat_elements method.

            So we need to simulate submission at a lower level I think.

            Also since many forms access optional param from their definition method we need to mock the submitted data before the form constructor is called.

            I have pushed a new version of the code here which addresses these issues : https://github.com/jamiepratt/moodle/compare/wip_MDL-40176_v3

            I'll hold off submitting a peer review request till I have more tests written, in case there are further changes needed.

            But if watchers have a minute I would be interested in their thoughts on these changes.

            Show
            jamiesensei Jamie Pratt added a comment - The first version of mock_submit is not going to work for many forms. Many forms access submitted data through optional_param or similar, including all forms that use the repeat_elements method. So we need to simulate submission at a lower level I think. Also since many forms access optional param from their definition method we need to mock the submitted data before the form constructor is called. I have pushed a new version of the code here which addresses these issues : https://github.com/jamiepratt/moodle/compare/wip_MDL-40176_v3 I'll hold off submitting a peer review request till I have more tests written, in case there are further changes needed. But if watchers have a minute I would be interested in their thoughts on these changes.
            Hide
            timhunt Tim Hunt added a comment -

            Looks OK. My one concern is, should you some-how save the original contents of $_FILES, $_POST, etc, before you overwrite them? Or should that be the responsibility of the caller, or of the PHPunit framework? At any rate, the PHPdoc comments need to make that part of the contract explicit.

            Show
            timhunt Tim Hunt added a comment - Looks OK. My one concern is, should you some-how save the original contents of $_FILES, $_POST, etc, before you overwrite them? Or should that be the responsibility of the caller, or of the PHPunit framework? At any rate, the PHPdoc comments need to make that part of the contract explicit.
            Hide
            jamiesensei Jamie Pratt added a comment -

            I submitted a change to the unit test framework to restore _GET, _POST and _FILES after each test here https://tracker.moodle.org/browse/MDL-40388

            Show
            jamiesensei Jamie Pratt added a comment - I submitted a change to the unit test framework to restore _GET, _POST and _FILES after each test here https://tracker.moodle.org/browse/MDL-40388
            Hide
            jamiesensei Jamie Pratt added a comment -

            Petr, I have you down as the peer reviewer. Can you take a look at the new code. Maybe we could get it in the next weekly release.

            Show
            jamiesensei Jamie Pratt added a comment - Petr, I have you down as the peer reviewer. Can you take a look at the new code. Maybe we could get it in the next weekly release.
            Hide
            skodak Petr Skoda added a comment -

            where is the patch? the link for pull diff does not work for me

            Show
            skodak Petr Skoda added a comment - where is the patch? the link for pull diff does not work for me
            Hide
            jamiesensei Jamie Pratt added a comment -

            Sorry Petr, the link is working now.

            Show
            jamiesensei Jamie Pratt added a comment - Sorry Petr, the link is working now.
            Hide
            skodak Petr Skoda added a comment -

            +1, submitting for itnegration

            Show
            skodak Petr Skoda added a comment - +1, submitting for itnegration
            Hide
            markn Mark Nelson added a comment -

            Thanks, this would be really handy for an issue I am working on atm.

            Show
            markn Mark Nelson added a comment - Thanks, this would be really handy for an issue I am working on atm.
            Hide
            damyon Damyon Wiese added a comment -

            I like this change. This is currently stopping us from adding more unit tests in many places. I will backport this to stables too as it will let us backport new unit tests that make use of it.

            Show
            damyon Damyon Wiese added a comment - I like this change. This is currently stopping us from adding more unit tests in many places. I will backport this to stables too as it will let us backport new unit tests that make use of it.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Jamie. I have needed this before so it is a good change.

            I have backported this to 24, 25, and master branches and added info to the upgrade.txt files. The reason for the backport is that this issue would block us from backporting unit tests in the future.

            Show
            damyon Damyon Wiese added a comment - Thanks Jamie. I have needed this before so it is a good change. I have backported this to 24, 25, and master branches and added info to the upgrade.txt files. The reason for the backport is that this issue would block us from backporting unit tests in the future.
            Hide
            dmonllao David Monllaó added a comment -

            Passing as unit tests are running in the CI server, please Jamie Pratt comment about it if it should not be passed like this

            Show
            dmonllao David Monllaó added a comment - Passing as unit tests are running in the CI server, please Jamie Pratt comment about it if it should not be passed like this
            Hide
            jamiesensei Jamie Pratt added a comment -

            David, I am not familiar with your Continuous Integration server and the use of it, but sounds like the unit tests are passing there as well as elsewhere, so all is good.

            Thanks.

            Show
            jamiesensei Jamie Pratt added a comment - David, I am not familiar with your Continuous Integration server and the use of it, but sounds like the unit tests are passing there as well as elsewhere, so all is good. Thanks.
            Hide
            damyon Damyon Wiese added a comment -

            a single bug fix
            a drop in a waterfall
            hear the mighty roar

            Thanks for your contribution!

            Show
            damyon Damyon Wiese added a comment - a single bug fix a drop in a waterfall hear the mighty roar Thanks for your contribution!
            Hide
            markn Mark Nelson added a comment -

            Imo, this does need to be documented somewhere. The description was enough for me to understand how to use the new functionality, but would nice to have a document to point to others who wish to add this to their unit tests.

            Show
            markn Mark Nelson added a comment - Imo, this does need to be documented somewhere. The description was enough for me to understand how to use the new functionality, but would nice to have a document to point to others who wish to add this to their unit tests.
            Hide
            timhunt Tim Hunt added a comment -

            Mark, isn't it lucky that http://docs.moodle.org/dev/Writing_PHPUnit_tests is a wiki. Anyone who understands this could document it. Even you

            At any rate, you are right, so thanks for adding the (dev_)docs required label.

            Show
            timhunt Tim Hunt added a comment - Mark, isn't it lucky that http://docs.moodle.org/dev/Writing_PHPUnit_tests is a wiki. Anyone who understands this could document it. Even you At any rate, you are right, so thanks for adding the (dev_)docs required label.
            Hide
            jamiesensei Jamie Pratt added a comment -
            Show
            jamiesensei Jamie Pratt added a comment - I made a start here : http://docs.moodle.org/dev/lib/formslib.php_Testing
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for adding some Dev docs, Jamie.

            Show
            salvetore Michael de Raadt added a comment - Thanks for adding some Dev docs, Jamie.

              People

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

                Dates

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