Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34889

PHP unit --drop should output some progress information

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Unit tests
    • Labels:

      Description

      I just had to wait about 10+ minutes wondering "Is this script doing anything?" which was not very nice.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            Not the most elegant code ever, but it makes things better for me. Would be nice if the progress_bar class worked in cli scripts.

            I have not yet worked out why drop table is so slow for me.

            Show
            timhunt Tim Hunt added a comment - Not the most elegant code ever, but it makes things better for me. Would be nice if the progress_bar class worked in cli scripts. I have not yet worked out why drop table is so slow for me.
            Hide
            timhunt Tim Hunt added a comment -

            Looks like a kown Posgres performance issue which might be fixed in 9.2: http://postgresql.1045698.n5.nabble.com/slow-dropping-of-tables-DropRelFileNodeBuffers-tas-td5710472.html

            Show
            timhunt Tim Hunt added a comment - Looks like a kown Posgres performance issue which might be fixed in 9.2: http://postgresql.1045698.n5.nabble.com/slow-dropping-of-tables-DropRelFileNodeBuffers-tas-td5710472.html
            Hide
            skodak Petr Skoda added a comment -

            Hmm, I am not a big friend of unconditional debug output, could we have some $verbose parameter in drop_site() method? I was considering adding it to the install phase too, but that would require changes in standard installation methods.

            Show
            skodak Petr Skoda added a comment - Hmm, I am not a big friend of unconditional debug output, could we have some $verbose parameter in drop_site() method? I was considering adding it to the install phase too, but that would require changes in standard installation methods.
            Hide
            timhunt Tim Hunt added a comment -

            Install is already verbose enough. I really don't see the need for an option here. Things should juts work for people wanting to run unit tests.

            Show
            timhunt Tim Hunt added a comment - Install is already verbose enough. I really don't see the need for an option here. Things should juts work for people wanting to run unit tests.
            Hide
            skodak Petr Skoda added a comment -

            I do want to make the installation verbosity optional, your change creates more work for me, sorry. Of course in case of phpunit scripts they would use verbose output by default.

            Show
            skodak Petr Skoda added a comment - I do want to make the installation verbosity optional, your change creates more work for me, sorry. Of course in case of phpunit scripts they would use verbose output by default.
            Hide
            skodak Petr Skoda added a comment -

            No low level api should echo stuff unconditionally, sorry, that is very, very wrong. Imagine one day we want xml output from this method. Ideally we should pass render instance to methods like this.

            Show
            skodak Petr Skoda added a comment - No low level api should echo stuff unconditionally, sorry, that is very, very wrong. Imagine one day we want xml output from this method. Ideally we should pass render instance to methods like this.
            Hide
            timhunt Tim Hunt added a comment -

            Oh, I see. You are saying that drop_site should not output by default. There should be a $displayprogress parameter, but it is OK for the CLI script to set that to true by default.

            Show
            timhunt Tim Hunt added a comment - Oh, I see. You are saying that drop_site should not output by default. There should be a $displayprogress parameter, but it is OK for the CLI script to set that to true by default.
            Hide
            timhunt Tim Hunt added a comment -

            Master branch updated. The code is getting more and more ugly, but, as a user, I still like the result.

            Show
            timhunt Tim Hunt added a comment - Master branch updated. The code is getting more and more ugly, but, as a user, I still like the result.
            Hide
            skodak Petr Skoda added a comment -

            +1, thanks

            Show
            skodak Petr Skoda added a comment - +1, thanks
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Petr. Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Thanks Petr. Submitting for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks! (23 & master).

            Silly question: Always I see those harcoded "\n" think if they shouldn't be PHP_EOLs or similar (or pass them trough some clilib facility).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (23 & master). Silly question: Always I see those harcoded "\n" think if they shouldn't be PHP_EOLs or similar (or pass them trough some clilib facility).
            Hide
            timhunt Tim Hunt added a comment -

            I have always just used \n, and it seems to work. Note I typically develop on Mac or Win. So, you may be right in theory, but in practice it is not necessary.

            Show
            timhunt Tim Hunt added a comment - I have always just used \n, and it seems to work. Note I typically develop on Mac or Win. So, you may be right in theory, but in practice it is not necessary.
            Hide
            fred Frédéric Massart added a comment -

            All good. Cheers!

            Show
            fred Frédéric Massart added a comment - All good. Cheers!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            YEAR!*

            CAF*, TOT!*

            • Your effort amazingly resulted. (unbelievable :-P)
            • Closing as fixed.
            • Tons of thanks.
            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Sep/12