Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.1
    • Component/s: Gradebook, Unit tests
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      1555

      Description

      The grades related unit tests could be better. Suggestions:

      1) test_grade_category_set_hidden() should test that hiding the category hides all grade_items within it. Not just the grade item associated with the category itself.

      2) Add tests to test grade calculation using the various category aggregation methods.

      3) Add tests to test grade calculation after some grade items have been hidden. These should include tests of what grades are displayed to student with "Hide totals if they contain hidden items" set to either hidden, show ex or show inc.

      Note that 2 and 3 may need to be combined to allow for easier testing of all category aggregation methods with and without hidden items.

      1. 20100922.patch
        14 kB
        Andrew Davis
      2. 20101111.patch
        51 kB
        Andrew Davis
      3. 20101203unittests.patch
        57 kB
        Andrew Davis
      4. MDL-24330unittests.patch
        51 kB
        Andrew Davis

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Attaching a patch that prevents the test tables being created, populated and dropped over and over. On my home machine this takes the execution time of the tests in a single file (/lib/grade/simpletest/testgradecategory.php) from 166 seconds down to 9 seconds.

          Note that it also breaks a bunch of the tests as each one was assuming a clean set of test data. These will need to be fixed but having the unit tests run in a reasonable time is key. I'm going to be adding a bunch of tests and using those tests to reassure myself of the correctness of the grade calculation algorithms. For that to really work the tests need to be able to run in a matter of minutes and not require being left running overnight.

          Show
          Andrew Davis added a comment - Attaching a patch that prevents the test tables being created, populated and dropped over and over. On my home machine this takes the execution time of the tests in a single file (/lib/grade/simpletest/testgradecategory.php) from 166 seconds down to 9 seconds. Note that it also breaks a bunch of the tests as each one was assuming a clean set of test data. These will need to be fixed but having the unit tests run in a reasonable time is key. I'm going to be adding a bunch of tests and using those tests to reassure myself of the correctness of the grade calculation algorithms. For that to really work the tests need to be able to run in a matter of minutes and not require being left running overnight.
          Hide
          Andrew Davis added a comment -

          Adding a link to the grade calculation bug that prompted me to improve the unit tests.

          Show
          Andrew Davis added a comment - Adding a link to the grade calculation bug that prompted me to improve the unit tests.
          Hide
          Andrew Davis added a comment -

          Attaching a patch that greatly improves grade unit test execution speeds. Rather than building up the test data before every single test then tearing it all down it builds the test data runs one test that has a series of sub tests then tears it down.

          To execute all of the tests in /lib/grade did take 28 minutes. Now it takes about 2 and a half.

          As the gradebook guy I would love to be able to run all of these tests in the time it takes me to go get a coffee instead of having to settle for running them during lunch or overnight.

          Note: still some tests to fix that were broken by this change.

          Show
          Andrew Davis added a comment - Attaching a patch that greatly improves grade unit test execution speeds. Rather than building up the test data before every single test then tearing it all down it builds the test data runs one test that has a series of sub tests then tears it down. To execute all of the tests in /lib/grade did take 28 minutes. Now it takes about 2 and a half. As the gradebook guy I would love to be able to run all of these tests in the time it takes me to go get a coffee instead of having to settle for running them during lunch or overnight. Note: still some tests to fix that were broken by this change.
          Hide
          Sam Marshall added a comment -

          Hi Andrew:

          I didn't look at the code but am fully in agreement with your approach. While it is not a 'pure' way to do unit tests, execution speed 10x improvement is much more valuable than purity in this case, and the only likely consequence is that breaking one test might cause later ones to break too; that's not the end of the world if it is detected quickly (ie people run full set of unit tests regularly - a lot more likely to happen if they don't take half an hour).

          When I asked Tim about it too, he reluctantly agreed; he did complain that the gradebook should have been developed in such a way that it was separated from the database (i.e. you could do unit tests without requiring db setup at all, entirely in RAM) but it is a bit late for that!

          One thing - in current HEAD the unit tests (for the whole system) now finally give a 'green' result . Please don't commit any changes to unit tests unless the unit tests (for whole system) still run green, i.e. I don't think you should commit this until you fix it.

          I have filed MDLSITE-1120 requesting that the nightly unit test run is made to work again, this should enable us to see daily whether unit tests are still working.

          That's all my opinion. Although I have recently led others here to make the unit tests run and pass for moodle 2 launch hopefully, this is probably a one time effort and I'm not the maintainer of unit tests in moodle! So I don't have any say if you want to do whatever.

          Show
          Sam Marshall added a comment - Hi Andrew: I didn't look at the code but am fully in agreement with your approach. While it is not a 'pure' way to do unit tests, execution speed 10x improvement is much more valuable than purity in this case, and the only likely consequence is that breaking one test might cause later ones to break too; that's not the end of the world if it is detected quickly (ie people run full set of unit tests regularly - a lot more likely to happen if they don't take half an hour). When I asked Tim about it too, he reluctantly agreed; he did complain that the gradebook should have been developed in such a way that it was separated from the database (i.e. you could do unit tests without requiring db setup at all, entirely in RAM) but it is a bit late for that! One thing - in current HEAD the unit tests (for the whole system) now finally give a 'green' result . Please don't commit any changes to unit tests unless the unit tests (for whole system) still run green, i.e. I don't think you should commit this until you fix it. I have filed MDLSITE-1120 requesting that the nightly unit test run is made to work again, this should enable us to see daily whether unit tests are still working. That's all my opinion. Although I have recently led others here to make the unit tests run and pass for moodle 2 launch hopefully, this is probably a one time effort and I'm not the maintainer of unit tests in moodle! So I don't have any say if you want to do whatever.
          Hide
          Andrew Davis added a comment -

          Attaching another patch. still has 3 tests failing plus some of the new tests commented out. As we're on the eve of the release of Moodle 2.0 RC 2 I'm temporarily putting this on hold to help with other bug fixing.

          Show
          Andrew Davis added a comment - Attaching another patch. still has 3 tests failing plus some of the new tests commented out. As we're on the eve of the release of Moodle 2.0 RC 2 I'm temporarily putting this on hold to help with other bug fixing.
          Hide
          Andrew Davis added a comment -

          updated patch. all old tests are now passing. new tests mostly. There are todos marking the spots where as yet unexplained stuff is happening.

          Show
          Andrew Davis added a comment - updated patch. all old tests are now passing. new tests mostly. There are todos marking the spots where as yet unexplained stuff is happening.
          Hide
          Petr Škoda added a comment -

          Hmmm, could you please evaluate how is this going to work in phpunit? If it is possible to do something like this there then my +1, if not possible then -3.

          Show
          Petr Škoda added a comment - Hmmm, could you please evaluate how is this going to work in phpunit? If it is possible to do something like this there then my +1, if not possible then -3.
          Hide
          Andrew Davis added a comment -

          Looks like this should be find in phpunit. It expects test methods to start with the word "test" so this sub-test structure should work fine.

          Its possible that we could eventually do away with the sub-tests and use shared fixtures. http://www.phpunit.de/manual/current/en/fixtures.html#fixtures.sharing-fixture We'd put the test data insertion and removal in setUpBeforeClass()and tearDownAfterClass(). That could however potentially introduce a problem in that Im not sure whether or not you are guaranteed of the order the tests will run in. Tests that alter the test data could cause problems if they are being run in an unpredictable order.

          Show
          Andrew Davis added a comment - Looks like this should be find in phpunit. It expects test methods to start with the word "test" so this sub-test structure should work fine. Its possible that we could eventually do away with the sub-tests and use shared fixtures. http://www.phpunit.de/manual/current/en/fixtures.html#fixtures.sharing-fixture We'd put the test data insertion and removal in setUpBeforeClass()and tearDownAfterClass(). That could however potentially introduce a problem in that Im not sure whether or not you are guaranteed of the order the tests will run in. Tests that alter the test data could cause problems if they are being run in an unpredictable order.
          Hide
          Andrew Davis added a comment -

          Committed. There will be some more unit test related work to do as part of MDL-11837

          Show
          Andrew Davis added a comment - Committed. There will be some more unit test related work to do as part of MDL-11837
          Hide
          Andrew Davis added a comment -

          reopening this as I didnt commit all of the code changes I made and now we're switching to git so I dont know what Im meant to do.

          Show
          Andrew Davis added a comment - reopening this as I didnt commit all of the code changes I made and now we're switching to git so I dont know what Im meant to do.
          Hide
          Andrew Davis added a comment -

          all code committed

          Show
          Andrew Davis added a comment - all code committed

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: