Moodle
  1. Moodle
  2. MDL-32961

improve donotcommit test perf by execution of native grep

    Details

    • Type: Task Task
    • 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

      in unix or osx:
      1/ run phpunit tests - cca 10s perf improvement expected on unix like operating systems with grep when nothing found
      2/ try to add DONOTCOMMIT to one file and run tests again, run tests - failure expected

      in windows:
      1/ Run phpunit tests and ensure they are still working

      Show
      in unix or osx: 1/ run phpunit tests - cca 10s perf improvement expected on unix like operating systems with grep when nothing found 2/ try to add DONOTCOMMIT to one file and run tests again, run tests - failure expected in windows: 1/ Run phpunit tests and ensure they are still working
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w20_MDL-32961_m23_phpunitperf

      Description

      • try native grep for DONOTCOMMIT

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Hmm, I don't think I like this as it seems a bit fragile. It depends on the environment setup, and I am not sure that it will always be true;

            1/ Is that --exclude parameter standard across all versions of grep?
            2/ We don't check if php can actually access this system command, maybe the path is broken or something like SELINUX is running which prevents access to those system comamnds

            More to the point I think these tests should be moved into the CI setup and out from phpunit like this as it doesn't really seem the right place for it.

            Show
            Dan Poltawski added a comment - Hmm, I don't think I like this as it seems a bit fragile. It depends on the environment setup, and I am not sure that it will always be true; 1/ Is that --exclude parameter standard across all versions of grep? 2/ We don't check if php can actually access this system command, maybe the path is broken or something like SELINUX is running which prevents access to those system comamnds More to the point I think these tests should be moved into the CI setup and out from phpunit like this as it doesn't really seem the right place for it.
            Hide
            Petr Skoda added a comment - - edited

            I did not introduce this do not commit check myself, it would have to grep for GRRR in my code.

            The code is not fragile because it verifies the exit codes, if it does not work for any reason (binary not available, missing option, etc.) it does not return 1 and normal PHP grep is done instead. It can be only faster if there are no hits imo.

            Show
            Petr Skoda added a comment - - edited I did not introduce this do not commit check myself, it would have to grep for GRRR in my code. The code is not fragile because it verifies the exit codes, if it does not work for any reason (binary not available, missing option, etc.) it does not return 1 and normal PHP grep is done instead. It can be only faster if there are no hits imo.
            Hide
            Petr Skoda added a comment -

            And we already do shell execution in our init.php and webrunner.php, if php can not execute shell commands the tests would not run at all.

            Show
            Petr Skoda added a comment - And we already do shell execution in our init.php and webrunner.php, if php can not execute shell commands the tests would not run at all.
            Hide
            Dan Poltawski added a comment -

            Ah, sorry. I did not see that if falls back to php grep. (And know you didn't introduce these tests, just commenting on that)

            Show
            Dan Poltawski added a comment - Ah, sorry. I did not see that if falls back to php grep. (And know you didn't introduce these tests, just commenting on that)
            Hide
            Dan Poltawski added a comment -

            Integrated thanks

            Show
            Dan Poltawski added a comment - Integrated thanks
            Hide
            Dan Poltawski added a comment -

            Passed - I have created an issue to remove this test though

            Show
            Dan Poltawski added a comment - Passed - I have created an issue to remove this test though
            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:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: