Moodle
  1. Moodle
  2. MDL-34808

Add phpunit tests to examview import format

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.2.5, 2.3.2, 2.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      GOAL OF THIS FIX

      The examview questions import format was repaired a few weeks aogo in MDL-34483. So here we are testing that :

      • nothing was broken by the changes done to add phpunit tests;
      • while adding the tests, it was discovered that not all matching questions were imported correctly, so we must test that now this problem is fixed.

      NOTICE

      All tests should be done with developer debugging enabled to verify that no warning or error is displayed.

      REDO THE TESTS FROM MDL-34483

      Go to Question Bank -> import.
      Ensure that there is no change on the results of the tests done in MDL-34483. This involve importing the question/format/examview/tests/fixtures/questions.examview.xml file using the "Examview" file format and testing that the 6 imported questions match the exact descriptions included in MDL-34483.

      TEST THE CORRECTED IMPORT OF MATCHING QUESTIONS

      import the examview_sample.xml file attached to this issue using the "Examview" format. 6 questions should be imported with no warning or error displayed.
      I suggest to concentrate on the matching question but all other questions should be OK too.
      Open the "Classify the animals." question for editing. You should see 4 subquestions :

      • question 1 (empty) answer insect
      • question 2 frog answer amphibian
      • question 3 newt answer amphibian
      • question 4 cat answer mammal

      Cancel editing and open the same question in preview window. You should see the question text : Classify the animals. and 3 lines with cat, frog and newt (the order may be different if shuffling is enabled as default on the website you are testing) and for each one a drop down menu with 3 choices : amphibian, mammal, insect. Try to answer both with all correct responses or some wrong ones and verify all is working as expected.
      Reopen the same question for editing but this time click on Save button without changing anything. Verify no warning or validation error is displayed.

      Show
      GOAL OF THIS FIX The examview questions import format was repaired a few weeks aogo in MDL-34483 . So here we are testing that : nothing was broken by the changes done to add phpunit tests; while adding the tests, it was discovered that not all matching questions were imported correctly, so we must test that now this problem is fixed. NOTICE All tests should be done with developer debugging enabled to verify that no warning or error is displayed. REDO THE TESTS FROM MDL-34483 Go to Question Bank -> import. Ensure that there is no change on the results of the tests done in MDL-34483 . This involve importing the question/format/examview/tests/fixtures/questions.examview.xml file using the "Examview" file format and testing that the 6 imported questions match the exact descriptions included in MDL-34483 . TEST THE CORRECTED IMPORT OF MATCHING QUESTIONS import the examview_sample.xml file attached to this issue using the "Examview" format. 6 questions should be imported with no warning or error displayed. I suggest to concentrate on the matching question but all other questions should be OK too. Open the "Classify the animals." question for editing. You should see 4 subquestions : question 1 (empty) answer insect question 2 frog answer amphibian question 3 newt answer amphibian question 4 cat answer mammal Cancel editing and open the same question in preview window. You should see the question text : Classify the animals. and 3 lines with cat, frog and newt (the order may be different if shuffling is enabled as default on the website you are testing) and for each one a drop down menu with 3 choices : amphibian, mammal, insect. Try to answer both with all correct responses or some wrong ones and verify all is working as expected. Reopen the same question for editing but this time click on Save button without changing anything. Verify no warning or validation error is displayed.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
    • Rank:
      43303

      Description

      Now that MDL-34483 is closed it would be good (as Dan Poltawski noted in MDL-34483) to add some phpunit tests to this import format, so that we can check it is still working in the future. Rick's example file could serve as a basis for this tests.

        Issue Links

          Activity

          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          You can assign this issue to me, if you agree, as it's a follow up of MDL-34483.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, You can assign this issue to me, if you agree, as it's a follow up of MDL-34483 .
          Hide
          Jean-Michel Vedrine added a comment -

          I commited phpunits tests. But I also somewhat refactored some things in the examview format. I know that I should have created a separate issue but it was a lot easier to modify the format while working on the tests when the tests revealed some weakness in the format.
          I will explain the changes I made in details in a further comment.

          Show
          Jean-Michel Vedrine added a comment - I commited phpunits tests. But I also somewhat refactored some things in the examview format. I know that I should have created a separate issue but it was a lot easier to modify the format while working on the tests when the tests revealed some weakness in the format. I will explain the changes I made in details in a further comment.
          Hide
          Jean-Michel Vedrine added a comment -

          Details of the changes made to the format :

          • qformat_examview now extends qformat_based_on_xml rather than qformat_default. This permits to remove some code duplication. This also means that the code can only be tested on top of the code for MDL_34738 where the qformat_based_on_xml class is created
          • added function cleaninput But now that this function is used in several import formats it should be better to move it in question/format.php in the qformat_based_on_xml class
          • switched some functions from protected to public so that I can call them from the tests
          • enterely refactored the import of match questions because the previous one was not working when question included some distractors of subquestions with same answers (I only saw this when trying the test with my favourite cat - frog - newt question). The method used is now the same as I use in other imports and (I think) it works in all cases.
          • removed some hardcoded strings ("correct" and "incorrect") and replaced them by call to get_string with existing lang strings
          • some arrays used during the import (during import of MCQ questions for instance) were created with keys imported from examview (they look like "choice-a", "choice-b", ...) these keys are of no use, so I removed them, even if the import was working with these keys. I only realized the use of these useless keys when writting the tests so rather than modifying the tests, I modified the format to create arrays with 0, 1, 2, ... keys as in any other format.
            I am quite sure my changes are right and that the examview is working a lot better for importing match questions and exactly as before for importing other questions types, but if this approach is rejected, I can separate this issue in 2 diffrents, one for improving the format and the other for adding phpunit tests.
          Show
          Jean-Michel Vedrine added a comment - Details of the changes made to the format : qformat_examview now extends qformat_based_on_xml rather than qformat_default. This permits to remove some code duplication. This also means that the code can only be tested on top of the code for MDL_34738 where the qformat_based_on_xml class is created added function cleaninput But now that this function is used in several import formats it should be better to move it in question/format.php in the qformat_based_on_xml class switched some functions from protected to public so that I can call them from the tests enterely refactored the import of match questions because the previous one was not working when question included some distractors of subquestions with same answers (I only saw this when trying the test with my favourite cat - frog - newt question). The method used is now the same as I use in other imports and (I think) it works in all cases. removed some hardcoded strings ("correct" and "incorrect") and replaced them by call to get_string with existing lang strings some arrays used during the import (during import of MCQ questions for instance) were created with keys imported from examview (they look like "choice-a", "choice-b", ...) these keys are of no use, so I removed them, even if the import was working with these keys. I only realized the use of these useless keys when writting the tests so rather than modifying the tests, I modified the format to create arrays with 0, 1, 2, ... keys as in any other format. I am quite sure my changes are right and that the examview is working a lot better for importing match questions and exactly as before for importing other questions types, but if this approach is rejected, I can separate this issue in 2 diffrents, one for improving the format and the other for adding phpunit tests.
          Hide
          Tim Hunt added a comment -

          Based on a quick review, this looks good.

          It is quite natural that when you write unit tests, you find bugs, and need to fix them, so no problem there.

          Shall we leave this until MDL-34738 is integrate next week, and then submit this? Or shall we try to submit this at the same time?

          Show
          Tim Hunt added a comment - Based on a quick review, this looks good. It is quite natural that when you write unit tests, you find bugs, and need to fix them, so no problem there. Shall we leave this until MDL-34738 is integrate next week, and then submit this? Or shall we try to submit this at the same time?
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to advance the workflow !!

          Show
          Jean-Michel Vedrine added a comment - I forgot to advance the workflow !!
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot also to ask you to peer review this !!

          Show
          Jean-Michel Vedrine added a comment - I forgot also to ask you to peer review this !!
          Hide
          Tim Hunt added a comment -

          See my comments above.

          Show
          Tim Hunt added a comment - See my comments above.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Is it possible to submit MDL-34738 and MDL-34808 together ? If yes that would be the best solution, I think.
          Do I need to make something special ? I don't think there is any intersection between the changed files between the 2 issues but I will verify.

          Show
          Jean-Michel Vedrine added a comment - - edited Is it possible to submit MDL-34738 and MDL-34808 together ? If yes that would be the best solution, I think. Do I need to make something special ? I don't think there is any intersection between the changed files between the 2 issues but I will verify.
          Hide
          Jean-Michel Vedrine added a comment -

          No intersection

          Show
          Jean-Michel Vedrine added a comment - No intersection
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to add that according to the codechecker the only problem is that there are 3 too long lines in the xml included in the test file, but I don't know if I must change them ?

          Show
          Jean-Michel Vedrine added a comment - I forgot to add that according to the codechecker the only problem is that there are 3 too long lines in the xml included in the test file, but I don't know if I must change them ?
          Hide
          Tim Hunt added a comment -

          It is possible, but you have to be careful. An example is http://tracker.moodle.org/browse/MDL-32705, notice how the diff URLs look like https://github.com/timhunt/moodle/compare/MDL-34728...MDL-32705, because I made the MDL-32705 branch follow on from the MDL-34728 branch. I also made the dependency clear in the comments.

          The way to do that is something like

          git checkout MDL-34808
          git rebase --onto MDL-34738 master

          We would also need testing instructions before this can be submitted for integration.

          Show
          Tim Hunt added a comment - It is possible, but you have to be careful. An example is http://tracker.moodle.org/browse/MDL-32705 , notice how the diff URLs look like https://github.com/timhunt/moodle/compare/MDL-34728...MDL-32705 , because I made the MDL-32705 branch follow on from the MDL-34728 branch. I also made the dependency clear in the comments. The way to do that is something like git checkout MDL-34808 git rebase --onto MDL-34738 master We would also need testing instructions before this can be submitted for integration.
          Hide
          Tim Hunt added a comment -

          Probably better to line-wrap the XML. Whitespace in XML is not very significant.

          Also, I just fixed MDL-32705 ready for next week, so it is no longer a good example of what I was talking about.

          Show
          Tim Hunt added a comment - Probably better to line-wrap the XML. Whitespace in XML is not very significant. Also, I just fixed MDL-32705 ready for next week, so it is no longer a good example of what I was talking about.
          Hide
          Jean-Michel Vedrine added a comment -

          The file to be used during the tests. Note this contains the exact same questions that are used dring phpunit tests.

          Show
          Jean-Michel Vedrine added a comment - The file to be used during the tests. Note this contains the exact same questions that are used dring phpunit tests.
          Hide
          Jean-Michel Vedrine added a comment -

          I still need to add a branch for moodle 2.3.
          I wonder if it is necessary to backport this to Moodle 2.2 ? phpunits tests are of no interest, and the fix of matching questions is quite minor (most matching questions used by teachers are one to one mappings so they are imported with no problem by the old code).

          Show
          Jean-Michel Vedrine added a comment - I still need to add a branch for moodle 2.3. I wonder if it is necessary to backport this to Moodle 2.2 ? phpunits tests are of no interest, and the fix of matching questions is quite minor (most matching questions used by teachers are one to one mappings so they are imported with no problem by the old code).
          Hide
          Tim Hunt added a comment -

          I suggest you back-port the changes to the format.php files, which are improvements, but don't worry about back-porting the tests.

          Show
          Tim Hunt added a comment - I suggest you back-port the changes to the format.php files, which are improvements, but don't worry about back-porting the tests.
          Hide
          Tim Hunt added a comment -

          Jean-Michel, are you going to re-base this fix, so we can submit it for integration too?

          Show
          Tim Hunt added a comment - Jean-Michel, are you going to re-base this fix, so we can submit it for integration too?
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Thanks for the reminder, I completely forgot that issue !!
          I don't know if I will be able to rebase and add 2.2 and 2.3 branches in time for this week, but I will try

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Thanks for the reminder, I completely forgot that issue !! I don't know if I will be able to rebase and add 2.2 and 2.3 branches in time for this week, but I will try
          Hide
          Jean-Michel Vedrine added a comment -

          Following Tim's advice I made branches for stable branches 2.2 and 2.3 but for 2.2 I only included the fix to match questions import and not the phpunits tests.

          Show
          Jean-Michel Vedrine added a comment - Following Tim's advice I made branches for stable branches 2.2 and 2.3 but for 2.2 I only included the fix to match questions import and not the phpunits tests.
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to say that I rebased too. Tim if you agree with the chnages, you can submit this to integration review.

          Show
          Jean-Michel Vedrine added a comment - I forgot to say that I rebased too. Tim if you agree with the chnages, you can submit this to integration review.
          Hide
          Tim Hunt added a comment -

          +1 from me. Submitting for integration. Thanks Jean-Michel, more good work.

          To INTEGRATORS: MDL-34738 must be integrated before this one.

          Show
          Tim Hunt added a comment - +1 from me. Submitting for integration. Thanks Jean-Michel, more good work. To INTEGRATORS: MDL-34738 must be integrated before this one.
          Hide
          Dan Poltawski added a comment -

          I've integrated this, thanks!

          Show
          Dan Poltawski added a comment - I've integrated this, thanks!
          Hide
          David Monllaó added a comment -

          I've failed it following strictly the testing instructions: "testing that the 6 imported questions match the exact descriptions in MDL-34483"

          Importing the examview_example.xml file, in the MDL-34483 test I've found this:

          • 3b) The last one is "none of these"
          • 3d) The 4 subquestion with empty question and the answer "Question 4" appears both in master and 22
          • 3e) There is a second answer "*" with feedback "Incorrect" in both branches

          I think that 3b is an error writting the testing instructions and 3d is solved (my opinion is derived from the MDL-34808 test -> matching question -> question 1 expected result) but I don't know about 3e one.

          Same results in 22 and master

          Show
          David Monllaó added a comment - I've failed it following strictly the testing instructions: "testing that the 6 imported questions match the exact descriptions in MDL-34483 " Importing the examview_example.xml file, in the MDL-34483 test I've found this: 3b) The last one is "none of these" 3d) The 4 subquestion with empty question and the answer "Question 4" appears both in master and 22 3e) There is a second answer "*" with feedback "Incorrect" in both branches I think that 3b is an error writting the testing instructions and 3d is solved (my opinion is derived from the MDL-34808 test -> matching question -> question 1 expected result) but I don't know about 3e one. Same results in 22 and master
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          Thanks a lot for this thoroug testing.
          My bad, I blindly said that results should be the same that in MDL-34483 forgetting to do the test myself and forgetting I had corrected bugs ! I should have been more carefull.
          Here is the details on what you have found :

          • 3b) Yes this is an error in writting the testing instructions (and it was not noticed testing MDL-34483) so I think the test is OK an maybe it would be necessary to update the instructions in MDL-34483 just in case it would be necessary to refer to these instructions in the future ? I suggest we consider this part as passed.
          • 3d) Yes I forgot to say that now the problem is solved and the matching question is inported as it should, so in fact the presence of Question 4 is expected. I suggest we also consider this part as passed.
          • 3e) Oops! My bad ! I changed my mind about the right way to import fill in the blanck (= shortanswer) questions during development and forgot to tell anybody and update the testing instructions. I now create a supplementary answer with the wildcard character * meaning "anything else", a 0% fraction grade and the incorrect feedback.

          This was done to improve conscistency with other import formats and I think it's the right thing to do, but maybe I should have discussed it in the forum before doing it ? But this format has been broken for many years and it's difficult to start a discussion.
          What can we do ? We can :

          • consider the test as passed because the question is imported as I now intend it to be.
          • restart development and revert the import of shortanswer the way it was before this change (easy done : this is just a few lines suppression both in the format's code and the corresponding phpunit test)
          • put this aside and ask users what they think of that change

          David and Tim what do you think of this ?

          Show
          Jean-Michel Vedrine added a comment - Hello David, Thanks a lot for this thoroug testing. My bad, I blindly said that results should be the same that in MDL-34483 forgetting to do the test myself and forgetting I had corrected bugs ! I should have been more carefull. Here is the details on what you have found : 3b) Yes this is an error in writting the testing instructions (and it was not noticed testing MDL-34483 ) so I think the test is OK an maybe it would be necessary to update the instructions in MDL-34483 just in case it would be necessary to refer to these instructions in the future ? I suggest we consider this part as passed. 3d) Yes I forgot to say that now the problem is solved and the matching question is inported as it should, so in fact the presence of Question 4 is expected. I suggest we also consider this part as passed. 3e) Oops! My bad ! I changed my mind about the right way to import fill in the blanck (= shortanswer) questions during development and forgot to tell anybody and update the testing instructions. I now create a supplementary answer with the wildcard character * meaning "anything else", a 0% fraction grade and the incorrect feedback. This was done to improve conscistency with other import formats and I think it's the right thing to do, but maybe I should have discussed it in the forum before doing it ? But this format has been broken for many years and it's difficult to start a discussion. What can we do ? We can : consider the test as passed because the question is imported as I now intend it to be. restart development and revert the import of shortanswer the way it was before this change (easy done : this is just a few lines suppression both in the format's code and the corresponding phpunit test) put this aside and ask users what they think of that change David and Tim what do you think of this ?
          Hide
          Tim Hunt added a comment -

          I think we should consider the test passed.

          I think you should now post in the quiz forum, telling people what has changed. If they object (which seems unlikely) we can open a new bug to resolve things later.

          Show
          Tim Hunt added a comment - I think we should consider the test passed. I think you should now post in the quiz forum, telling people what has changed. If they object (which seems unlikely) we can open a new bug to resolve things later.
          Hide
          David Monllaó added a comment -

          Hi Jean-Michel,

          For me there is no problem in passing it; both of you know better than me what's the best option, so let's wait for Tim's feedback.

          Thanks

          Show
          David Monllaó added a comment - Hi Jean-Michel, For me there is no problem in passing it; both of you know better than me what's the best option, so let's wait for Tim's feedback. Thanks
          Hide
          David Monllaó added a comment -

          Passing it as discussed. Thanks Dan for enable it again

          Show
          David Monllaó added a comment - Passing it as discussed. Thanks Dan for enable it again
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello David and Tim,
          So if Tim agree, I suggest to consider the test as passed.
          I will upgrade the testing instructions of MDL-34483 for "none of these" and start a discussion in the forum for shortanswer import.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello David and Tim, So if Tim agree, I suggest to consider the test as passed. I will upgrade the testing instructions of MDL-34483 for "none of these" and start a discussion in the forum for shortanswer import.
          Hide
          David Monllaó added a comment -

          Thanks Jean-Michel

          Show
          David Monllaó added a comment - Thanks Jean-Michel
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks David, Dan and Tim.

          Show
          Jean-Michel Vedrine added a comment - Thanks David, Dan and Tim.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: