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:

      Description

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

        Gliffy Diagrams

          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: