Details
-
Type:
Improvement
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 2.5
-
Component/s: Forms Library, Unit tests
-
Labels:
-
Testing Instructions:
-
Affected Branches:MOODLE_25_STABLE
-
Fixed Branches:MOODLE_24_STABLE, MOODLE_25_STABLE
-
Epic Link:
-
Pull from Repository:
-
Pull Master Branch:wip_
MDL-40176_v4 -
Pull Master Diff URL:
-
Documentation link:
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
Attachments
Issue Links
Activity
- All
- Comments
- Work Log
- History
- Activity
- Links Hierarchy
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.
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.
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 {.
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.
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.
+1, btw I could not go to AU, I am still stuck here in CZ
Oh. That sucks. Thanks for the review.
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.
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.
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
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.
where is the patch? the link for pull diff does not work for me
Sorry Petr, the link is working now.
+1, submitting for itnegration
Thanks, this would be really handy for an issue I am working on atm. 
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.
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.
Passing as unit tests are running in the CI server, please Jamie Pratt comment about it if it should not be passed like this
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.
a single bug fix
a drop in a waterfall
hear the mighty roar
Thanks for your contribution!
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.
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.
I made a start here : http://docs.moodle.org/dev/lib/formslib.php_Testing
Thanks for adding some Dev docs, Jamie.
Yes. Nice idea.