Moodle
  1. Moodle
  2. MDL-32016

Unit tests contain hard coded file hash

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Unit tests
    • Labels:
    • Testing Instructions:
      Hide

      Replace the image backup/converter/moodle1/simpletest/files/icon.gif with another file of the same name. Run unit tests (to make them run faster limit to the path backup/converter/)

      Expect:

      • tests still pass

      What happens:

      • tests fails
      Show
      Replace the image backup/converter/moodle1/simpletest/files/icon.gif with another file of the same name. Run unit tests (to make them run faster limit to the path backup/converter/) Expect: tests still pass What happens: tests fails
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      38688

      Description

      In theory this could break tests in the future, I propose calculating the hash dynamically.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for contributing this, Aaron, but it should go through peer review before going to integration.

        Show
        Michael de Raadt added a comment - Thanks for contributing this, Aaron, but it should go through peer review before going to integration.
        Hide
        Michael de Raadt added a comment -

        Actually, I found it was not possible to go from the status "Re-opened" to "Request peer review". I've adjusted the workflow so this is now possible and it should appear in the list of issues to peer review.

        Show
        Michael de Raadt added a comment - Actually, I found it was not possible to go from the status "Re-opened" to "Request peer review". I've adjusted the workflow so this is now possible and it should appear in the list of issues to peer review.
        Hide
        Michael de Raadt added a comment -

        Setting Aaron as assignee as he has done he work.

        Show
        Michael de Raadt added a comment - Setting Aaron as assignee as he has done he work.
        Hide
        Aparup Banerjee added a comment -

        This looks good to me!
        Thanks for the patch!

        ------

        ps: i know its in the code above but i might be tempted to put the

        "$CFG->tempdir/backup/$this->tempdir/moddata/unittest/4/icon.gif" 
        

        as

        $CFG->tempdir . '/backup/' . $this->tempdir . '/moddata/unittest/4/icon.gif'

        just for clarity's sake (imo)

        Show
        Aparup Banerjee added a comment - This looks good to me! Thanks for the patch! ------ ps: i know its in the code above but i might be tempted to put the "$CFG->tempdir/backup/$ this ->tempdir/moddata/unittest/4/icon.gif" as $CFG->tempdir . '/backup/' . $ this ->tempdir . '/moddata/unittest/4/icon.gif' just for clarity's sake (imo)
        Hide
        Aaron Barnes added a comment -

        Hi Aprarup,

        Is that preferred in Moodle over
        "{$CFG->tempdir}/backup/{$this->tempdir}/moddata/unittest/4/icon.gif"
        ?

        Cheers,
        Aaron

        Show
        Aaron Barnes added a comment - Hi Aprarup, Is that preferred in Moodle over "{$CFG->tempdir}/backup/{$this->tempdir}/moddata/unittest/4/icon.gif" ? Cheers, Aaron
        Hide
        Aparup Banerjee added a comment -

        personally i like the dots - it helps clarity for me and we can always put long strings on multiple lines, but i don't think theres anything stopping curly brace uses

        The coding style says http://docs.moodle.org/dev/Coding_style#String_concatenation

        ps: (a little above there is a use of the {$name} which is a little confusing too)

        Show
        Aparup Banerjee added a comment - personally i like the dots - it helps clarity for me and we can always put long strings on multiple lines, but i don't think theres anything stopping curly brace uses The coding style says http://docs.moodle.org/dev/Coding_style#String_concatenation ps: (a little above there is a use of the {$name} which is a little confusing too)
        Hide
        Aaron Barnes added a comment -

        Updated patch

        Show
        Aaron Barnes added a comment - Updated patch
        Hide
        Aparup Banerjee added a comment -

        looked good, still does! sending it up for integration.

        Show
        Aparup Banerjee added a comment - looked good, still does! sending it up for integration.
        Hide
        Sam Hemelryk added a comment -

        Thanks guys, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks guys, this has been integrated now
        Hide
        Rajesh Taneja added a comment -

        Test pass after replacing icon.gif

        Thanks for fixing this, Aaron.

        Show
        Rajesh Taneja added a comment - Test pass after replacing icon.gif Thanks for fixing this, Aaron.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        U P S T R E A M I Z E D !

        Many thanks for the hard work, closing this as fixed.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: