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

          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