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
    • Rank:
      40078

      Description

      • try native grep for DONOTCOMMIT

        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 Škoda 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 Škoda 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 Škoda 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 Škoda 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: