Moodle
  1. Moodle
  2. MDL-34889

PHP unit --drop should output some progress information

    Details

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

      Description

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

        Activity

        Hide
        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
        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
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        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
        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
        Petr Škoda added a comment -

        +1, thanks

        Show
        Petr Škoda added a comment - +1, thanks
        Hide
        Tim Hunt added a comment -

        Thanks Petr. Submitting for integration.

        Show
        Tim Hunt added a comment - Thanks Petr. Submitting for integration.
        Hide
        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
        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
        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
        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
        Frédéric Massart added a comment -

        All good. Cheers!

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

        YEAR!*

        CAF*, TOT!*

        • Your effort amazingly resulted. (unbelievable :-P)
        • Closing as fixed.
        • Tons of thanks.
        Show
        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: