Moodle
  1. Moodle
  2. MDL-24905 Unit tests fail
  3. MDL-24980

lib/simpletest/testportfolioaddbutton.php unit test fails (needs migrating)

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: None
    • Component/s: Unit tests
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      1641
    1. MDL-24980.20101108 1511.patch
      2 kB
      Colin Chambers
    2. MDL-24980.20101109 1525.patch
      2 kB
      Colin Chambers
    3. MDL-24980.patch
      2 kB
      Colin Chambers

      Activity

      Hide
      Colin Chambers added a comment -

      attached patch file. Converted to use UnitTestCaseUsingDatabase. The test runs and fails because 2 formats are given but 3 are returned. The extra being leap2a.

      Reading the set_formats method in question I wasn't sure if this is correct behavious or not. Added nikck connaught as watcher since he wrote the original test.

      I was expecting the set_formats to set a specific list and thus you get back what you set. this is what the code comment describes. This isn't the case though. What's preferred?

      Show
      Colin Chambers added a comment - attached patch file. Converted to use UnitTestCaseUsingDatabase. The test runs and fails because 2 formats are given but 3 are returned. The extra being leap2a. Reading the set_formats method in question I wasn't sure if this is correct behavious or not. Added nikck connaught as watcher since he wrote the original test. I was expecting the set_formats to set a specific list and thus you get back what you set. this is what the code comment describes. This isn't the case though. What's preferred?
      Hide
      Sam Marshall added a comment -

      I committed this patch now (sorry for not doing it last week).

      Show
      Sam Marshall added a comment - I committed this patch now (sorry for not doing it last week).
      Hide
      Colin Chambers added a comment -

      Penny,

      could you confirmed what you'd like the test to check for. 2 formats or 3. Either the test has the wrong expectation or the code needs to be changed to meet the test. Not sure which moodle wants.

      Show
      Colin Chambers added a comment - Penny, could you confirmed what you'd like the test to check for. 2 formats or 3. Either the test has the wrong expectation or the code needs to be changed to meet the test. Not sure which moodle wants.
      Hide
      Penny Leach added a comment -

      Argh.

      That code is super out of date and the way formats works has now completely changed, as what is returned is a combination of what is explicitly set in the button, and what is set in the static method of the export class. In some cases they conflict, in which case the button wins. In this case, neither file or image conflict with leap2a, which is why all three are returned.

      I'm not sure there's really a sensible way to test this, unless you compare it with what the assignment_portfolio_caller::base_supported_formats returns.

      Show
      Penny Leach added a comment - Argh. That code is super out of date and the way formats works has now completely changed, as what is returned is a combination of what is explicitly set in the button, and what is set in the static method of the export class. In some cases they conflict, in which case the button wins. In this case, neither file or image conflict with leap2a, which is why all three are returned. I'm not sure there's really a sensible way to test this, unless you compare it with what the assignment_portfolio_caller::base_supported_formats returns.
      Hide
      Colin Chambers added a comment -

      Attached patch gets the tests passing according to pennys comments. I've added a little commenting to the test for future reference to anyone developing more tests.

      while the tests now pass I can't be sure that the logic used to determine the expected values to check for exactly matches the code since I haven't gone in depth.

      Are you happy with this solution or shall we just remove the test for now or come up with a better test instead.

      Show
      Colin Chambers added a comment - Attached patch gets the tests passing according to pennys comments. I've added a little commenting to the test for future reference to anyone developing more tests. while the tests now pass I can't be sure that the logic used to determine the expected values to check for exactly matches the code since I haven't gone in depth. Are you happy with this solution or shall we just remove the test for now or come up with a better test instead.
      Hide
      Penny Leach added a comment -

      I don't think that's the best way to test this.

      There are three metrics for doing the format detection (what you're simplifying to array_unique here)

      1. Uniqueness
      2. Conflicts (eg plain html format conflicts with "rich" html format (has attachments)
      3. Inheritance (eg "pdf" is more specific than "file" so "file" is removed)

      However, I'm not sure of the best way, except to manually call portfolio_supported_formats or whatever the helper is, and make sure we get the expected result.

      Show
      Penny Leach added a comment - I don't think that's the best way to test this. There are three metrics for doing the format detection (what you're simplifying to array_unique here) 1. Uniqueness 2. Conflicts (eg plain html format conflicts with "rich" html format (has attachments) 3. Inheritance (eg "pdf" is more specific than "file" so "file" is removed) However, I'm not sure of the best way, except to manually call portfolio_supported_formats or whatever the helper is, and make sure we get the expected result.
      Hide
      Colin Chambers added a comment -

      I wondered about the complexity of the test. I'd expect to break down each part and test each individually. Rather than all at once.

      I've attached patch that implements what you agreed earlier with sam

      Procedure for portfolio unit tests (agreed with Penny):

      1. Comment out all the tests (so that running them will give 0 passes, 0 fails, 0 exceptions).

      2. Near top of file add this comment:

      /*

      • TODO: The portfolio unit tests were obselete and did not work.
      • They have been commented out so that they do not break the
      • unit tests in Moodle 2.
        *
      • At some point:
      • 1. These tests should be audited to see which ones were valuable.
      • 2. The useful ones should be rewritten using the current standards
      • for writing test cases.
        *
      • This might be left until Moodle 2.1 when the test case framework
      • is due to change.
        */

      3. If there are specific warnings about a particular test (ie 'this test potentially nuked the real database'), you can add those at the end of the comment.

      I've left in the changes I made to get it using the new UnitTestCaseUsingDatabase class and then commented out the test.

      The test report now reads 0/1 test cases complete: 0 passes, 0 fails and 0 exceptions.

      Hope this is good enough for now.

      Show
      Colin Chambers added a comment - I wondered about the complexity of the test. I'd expect to break down each part and test each individually. Rather than all at once. I've attached patch that implements what you agreed earlier with sam Procedure for portfolio unit tests (agreed with Penny): 1. Comment out all the tests (so that running them will give 0 passes, 0 fails, 0 exceptions). 2. Near top of file add this comment: /* TODO: The portfolio unit tests were obselete and did not work. They have been commented out so that they do not break the unit tests in Moodle 2. * At some point: 1. These tests should be audited to see which ones were valuable. 2. The useful ones should be rewritten using the current standards for writing test cases. * This might be left until Moodle 2.1 when the test case framework is due to change. */ 3. If there are specific warnings about a particular test (ie 'this test potentially nuked the real database'), you can add those at the end of the comment. I've left in the changes I made to get it using the new UnitTestCaseUsingDatabase class and then commented out the test. The test report now reads 0/1 test cases complete: 0 passes, 0 fails and 0 exceptions. Hope this is good enough for now.
      Hide
      Sam Marshall added a comment -

      I committed the patch, so this test is now commented. Thanks Colin.

      Show
      Sam Marshall added a comment - I committed the patch, so this test is now commented. Thanks Colin.
      Hide
      Penny Leach added a comment -

      double thumbs up, thanks guys

      Show
      Penny Leach added a comment - double thumbs up, thanks guys

        People

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

          Dates

          • Created:
            Updated:
            Resolved: