Moodle
  1. Moodle
  2. MDL-36206

Create Unit Test for Pagination bar

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Libraries
    • Labels:
    • Rank:
      44986

      Description

      See MDL-30316 for details about the need for a unit test.

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          I've created what I think is the ideal unit test for this. Just need to wait til the linked issue is integrated before this one can be tested pre-integration.

          Show
          Jason Fowler added a comment - I've created what I think is the ideal unit test for this. Just need to wait til the linked issue is integrated before this one can be tested pre-integration.
          Hide
          Jason Fowler added a comment -

          Testing the unit test passed, pushing for peer review

          Show
          Jason Fowler added a comment - Testing the unit test passed, pushing for peer review
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,
          This will need some more work. Here are a few things I noticed:-

          1. This needs testing instructions(or simply a mention no testing required)
          2. You might want to backport this
          3. underscores are not allowed in variable names http://docs.moodle.org/dev/Coding_style#Variables
          4. I am not sure what is the scope of this issue but if you are testing the pagination_bar, you should be testing all possible use cases, not just the valid ones. That means testing for cases which throws coding_exceptions
          5. Can you please justify the use of this line:-
            var_dump($pbar_a->pagelinks);
            
          6. Your unit tests are failing. Here is the out put
            Starting test 'paging_bar_test::test_prepare'.                                                                                                                                                
            E                                                                                                                                                                                             
            
            Time: 0 seconds, Memory: 86.50Mb                                                                                                                                                              
            
            There was 1 error:                                                                                                                                                                            
            
            1) paging_bar_test::test_prepare                                                                                                                                                              
            Missing argument 2 for renderer_base::__construct(), called in /var/www/stable/master/moodle/lib/tests/outputcomponents_test.php on line 425 and defined                                      
            
            /var/www/stable/master/moodle/lib/outputrenderers.php:80                                                                                                                                      
            /var/www/stable/master/moodle/lib/tests/outputcomponents_test.php:425                                                                                                                         
            /var/www/stable/master/moodle/lib/phpunit/classes/advanced_testcase.php:76   
            
          7. You cannot have more than one test class in phpunit per file

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, This will need some more work. Here are a few things I noticed:- This needs testing instructions(or simply a mention no testing required) You might want to backport this underscores are not allowed in variable names http://docs.moodle.org/dev/Coding_style#Variables I am not sure what is the scope of this issue but if you are testing the pagination_bar, you should be testing all possible use cases, not just the valid ones. That means testing for cases which throws coding_exceptions Can you please justify the use of this line:- var_dump($pbar_a->pagelinks); Your unit tests are failing. Here is the out put Starting test 'paging_bar_test::test_prepare'. E Time: 0 seconds, Memory: 86.50Mb There was 1 error: 1) paging_bar_test::test_prepare Missing argument 2 for renderer_base::__construct(), called in /var/www/stable/master/moodle/lib/tests/outputcomponents_test.php on line 425 and defined /var/www/stable/master/moodle/lib/outputrenderers.php:80 /var/www/stable/master/moodle/lib/tests/outputcomponents_test.php:425 /var/www/stable/master/moodle/lib/phpunit/classes/advanced_testcase.php:76 You cannot have more than one test class in phpunit per file Thanks
          Hide
          Jason Fowler added a comment -

          Master only, will deal with the other problems you mentioned soon though.

          Show
          Jason Fowler added a comment - Master only, will deal with the other problems you mentioned soon though.
          Hide
          Jason Fowler added a comment -

          As for the test class in one phpunit file, have a look at outputlib_test.php that has a few classes in their.

          Show
          Jason Fowler added a comment - As for the test class in one phpunit file, have a look at outputlib_test.php that has a few classes in their.
          Hide
          Ankit Agarwal added a comment -

          Just to note for records, based on discussion on dev chat and other things, not to have more than one testclass per file.
          http://docs.moodle.org/dev/Writing_PHPUnit_tests#Testcase_classes

          The patch looks good now.
          Thanks

          Show
          Ankit Agarwal added a comment - Just to note for records, based on discussion on dev chat and other things, not to have more than one testclass per file. http://docs.moodle.org/dev/Writing_PHPUnit_tests#Testcase_classes The patch looks good now. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Damyon Wiese added a comment -

          Thanks Jason, this has been pushed to master, 24 and 23 branches.

          I cherry-picked this back to 2.3 because we like unit tests covering as much code as possible.

          I also added a commit to fix some trailing whitespace.

          Show
          Damyon Wiese added a comment - Thanks Jason, this has been pushed to master, 24 and 23 branches. I cherry-picked this back to 2.3 because we like unit tests covering as much code as possible. I also added a commit to fix some trailing whitespace.
          Hide
          Mark Nelson added a comment -

          Ran the PHP Unit test for the file that had been changed. No issues found.

          Show
          Mark Nelson added a comment - Ran the PHP Unit test for the file that had been changed. No issues found.
          Hide
          Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon

            People

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

              Dates

              • Created:
                Updated:
                Resolved: