Moodle
  1. Moodle
  2. MDL-40176

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

    Details

    • Rank:
      50921

      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);
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Yes. Nice idea.

          Show
          Michael de Raadt added a comment - Yes. Nice idea.
          Hide
          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
          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
          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
          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
          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 Škoda, 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
          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 Škoda , 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
          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
          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
          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
          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
          Petr Škoda added a comment -

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

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

          Oh. That sucks. Thanks for the review.

          Show
          Tim Hunt added a comment - Oh. That sucks. Thanks for the review.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Petr Škoda added a comment -

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

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

          Sorry Petr, the link is working now.

          Show
          Jamie Pratt added a comment - Sorry Petr, the link is working now.
          Hide
          Petr Škoda added a comment -

          +1, submitting for itnegration

          Show
          Petr Škoda added a comment - +1, submitting for itnegration
          Hide
          Mark Nelson added a comment -

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

          Show
          Mark Nelson added a comment - Thanks, this would be really handy for an issue I am working on atm.
          Hide
          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 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 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 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
          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
          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
          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
          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 Wiese added a comment -

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

          Thanks for your contribution!

          Show
          Damyon Wiese added a comment - a single bug fix a drop in a waterfall hear the mighty roar Thanks for your contribution!
          Hide
          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
          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
          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
          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
          Jamie Pratt added a comment -
          Show
          Jamie Pratt added a comment - I made a start here : http://docs.moodle.org/dev/lib/formslib.php_Testing
          Hide
          Michael de Raadt added a comment -

          Thanks for adding some Dev docs, Jamie.

          Show
          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: