-
Notifications
You must be signed in to change notification settings - Fork 2.2k
t: run tests using prove #3125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
t: run tests using prove #3125
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By default, the prove(1) program looks for tests to run in the 't' directory at the repository root, so let's put existing tests there in order to remove a non-default argument from prove.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Aug 30, 2024
In commit c591ff7 of PR git-lfs#3125 the GIT_LFS_NO_TEST_COUNT environment variable was introduced for use with our test suites. When this variable was set to a non-empty value, it indicated to the setup() and shutdown() test helper shell functions that they should skip running the lfstest-count-tests utility program entirely. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem in part by not counting the number of running test scripts on Windows. (We also introduced the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, in commit 9b73c80 of the same PR, to further address the problem.) When running our test suite on Windows, we set the GIT_LFS_NO_TEST_COUNT to "1", which causes the individual test suites to skip invoking lfstest-count-tests entirely. Instead, the program is run just once in the "test" t/Makefile target before executing the test suites with the "prove" command, and then once again afterwards, both times by explicitly resetting the GIT_LFS_NO_TEST_COUNT variable to an empty string value. These two invocations cause the lfstest-count-tests program to start the lfstest-gitserver program before all the test suites were executed and then stop it once they had all finished. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can remove this environment variable and the test suite features it controls. We change our script/cibuild script to no longer set the GIT_LFS_NO_TEST_COUNT variable, and we revise the conditionals in the setup() and shutdown() test helper shell functions so they always call the lfstest-count-tests program, rather than only doing so if the GIT_LFS_NO_TEST_COUNT variable is unset or empty. We also change the "test" target's recipe in our t/Makefile file to not unset GIT_LFS_NO_TEST_COUNT when running lfstest-count-tests before and after it runs the "prove" command. Finally, as we have already removed the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable and all support for it, in a previous commit in this PR, that allows us now to also eliminate the unset_vars() shell functions in some of our t/t-*.sh test scripts, which was used to unset both that variable and the GIT_LFS_NO_TEST_COUNT variable before running the tests in those scripts. We added this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Aug 30, 2024
In commit e1c2c8a of PR git-lfs#1107 we revised the test to use a separate test directory, either a temporary one we create or one specified in advance in a GIT_LFS_TEST_DIR environment variable. If that variable is empty or unset, we populate it with the output of a "mktemp -d" command with a directory name pattern of "git-lfs_TEMP". In either case, we then create all of our test repositories in subdirectories of the location given by GIT_LFS_TEST_DIR. If we create a directory because GIT_LFS_TEST_DIR is empty or unset, we also set the RM_GIT_LFS_TEST_DIR variable with a flag value of "yes". As of commit e1c2c8a, we would then check that variable in the shutdown() shell function, and if it was set to "yes", we would then remove the directory specified by the GIT_LFS_TEST_DIR variable if it matched the pattern we use for temporary directories and the KEEPTRASH variable was empty or unset. This ensured that, in the normal case, we removed all of our test artifacts after completing the test suite. In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. In that commmit, the "test" target of the t/Makefile file was written so it first loads the t/testenv.sh script and then runs the setup() shell function, which calls "lfstest-count-tests increment", which starts the lfstest-gitserver program because it increments the test count from zero to one. Every test script run by the "prove" command then calls setup() again, which further increments the test count, and then as it exits it calls the shutdown() shell function, which runs the "lfstest-count-tests decrement" command. That never decrements the test count below one, however, because of the initial call to setup() prior to running "prove". Finally, the "test" Makefile target runs shutdown() after "prove" is complete, which decrements the test count to zero, at which point lfstest-count-tests sends a shutdown POST HTTP request to the running lfstest-gitserver instance. In this design, though, each individual test script loads the t/testenv.sh script, and the script is directly loaded twice by the "test" Makefile target's recipe. In each case, it script runs in a separate shell session, and so does not find a GIT_LFS_TEST_DIR environment variable (unless one is set in the user's shell session when running "make test"). Thus each invocation of t/testenv.sh results in the creation of a new temporary test directory. Commit 1926e94 also removed the logic from the shutdown() shell function which handled the deletion of the directory specified by the GIT_LFS_TEST_DIR variable, if the RM_GIT_LFS_TEST_DIR variable was set to "yes" and we had clearly created a temporary directory to populate GIT_LFS_TEST_DIR. The consequence is that when we run an individual test script by itself, it leaves a single temporary test directory behind, and when we run the "make test" Makefile target, every t/t-*.sh script leaves a test directory behind, plus two additional ones created when the recipe loads the t/testenv.sh script directly. We can improve this situation by restoring the logic to clean up the directory specified by the GIT_LFS_TEST_DIR variable (if the RM_GIT_LFS_TEST_DIR variable contains "yes" and the other conditions described above are met), and then adjusting our "test" target's recipe so it runs all of its key steps in a single Bash shell session. We also make sure to export the GIT_LFS_TEST_DIR and RM_GIT_LFS_TEST_DIR variables into the environment, so that when t/testenv.sh is loaded by the recipe the first time, it creates a temporary directory and exports those variables, which ensures they are then available to all the subsequent test scripts and the recipe's final call to shutdown(). By making these changes, we ensure that in the normal case, when GIT_LFS_TEST_DIR is empty or unset and we run "make test", we create only a single temporary test directory, use it in all the individual test scripts, and then remove it completely afterwards.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Aug 31, 2024
In commit e1c2c8a of PR git-lfs#1107 we revised the test to use a separate test directory, either a temporary one we create or one specified in advance in a GIT_LFS_TEST_DIR environment variable. If that variable is empty or unset, we populate it with the output of a "mktemp -d" command with a directory name pattern of "git-lfs_TEMP". In either case, we then create all of our test repositories in subdirectories of the location given by GIT_LFS_TEST_DIR. If we create a directory because GIT_LFS_TEST_DIR is empty or unset, we also set the RM_GIT_LFS_TEST_DIR variable with a flag value of "yes". As of commit e1c2c8a, we would then check that variable in the shutdown() shell function, and if it was set to "yes", we would then remove the directory specified by the GIT_LFS_TEST_DIR variable if it matched the pattern we use for temporary directories and the KEEPTRASH variable was empty or unset. This ensured that, in the normal case, we removed all of our test artifacts after completing the test suite. In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. In that commmit, the "test" target of the t/Makefile file was written so it first loads the t/testenv.sh script and then runs the setup() shell function, which calls "lfstest-count-tests increment", which starts the lfstest-gitserver program because it increments the test count from zero to one. Every test script run by the "prove" command then calls setup() again, which further increments the test count, and then as it exits it calls the shutdown() shell function, which runs the "lfstest-count-tests decrement" command. That never decrements the test count below one, however, because of the initial call to setup() prior to running "prove". Finally, the "test" Makefile target runs shutdown() after "prove" is complete, which decrements the test count to zero, at which point lfstest-count-tests sends a shutdown POST HTTP request to the running lfstest-gitserver instance. In this design, though, each individual test script loads the t/testenv.sh script, and the script is directly loaded twice by the "test" Makefile target's recipe. In each case, it script runs in a separate shell session, and so does not find a GIT_LFS_TEST_DIR environment variable (unless one is set in the user's shell session when running "make test"). Thus each invocation of t/testenv.sh results in the creation of a new temporary test directory. Commit 1926e94 also removed the logic from the shutdown() shell function which handled the deletion of the directory specified by the GIT_LFS_TEST_DIR variable, if the RM_GIT_LFS_TEST_DIR variable was set to "yes" and we had clearly created a temporary directory to populate GIT_LFS_TEST_DIR. The consequence is that when we run an individual test script by itself, it leaves a single temporary test directory behind, and when we run the "make test" Makefile target, every t/t-*.sh script leaves a test directory behind, plus two additional ones created when the recipe loads the t/testenv.sh script directly. We can improve this situation by restoring the logic to clean up the directory specified by the GIT_LFS_TEST_DIR variable (if the RM_GIT_LFS_TEST_DIR variable contains "yes" and the other conditions described above are met), and then adjusting our "test" target's recipe so it runs all of its key steps in a single Bash shell session. We also make sure to export the GIT_LFS_TEST_DIR and RM_GIT_LFS_TEST_DIR variables into the environment, so that when t/testenv.sh is loaded by the recipe the first time, it creates a temporary directory and exports those variables, which ensures they are then available to all the subsequent test scripts and the recipe's final call to shutdown(). By making these changes, we ensure that in the normal case, when GIT_LFS_TEST_DIR is empty or unset and we run "make test", we create only a single temporary test directory, use it in all the individual test scripts, and then remove it completely afterwards. In our t/t-env.sh and t/t-workflow.sh test scripts, we check the output of the "git lfs env" command in a number of tests, comparing it to the values we expect based on the test conditions, including the values of all environment variables whose names begin with the "GIT_" prefix. As we now export the GIT_LFS_TEST_DIR variable into the environment, it is among these environment variables, which causes the tests to fail on Windows unless we make one additional change. We run our CI test suite on Windows using the Git Bash environment provided by the Git for Windows project, which is in turn based on the MSYS2 environment. In this context, the mktemp(1) command returns a path beginning with /tmp, so that is what is stored in our GIT_LFS_TEST_DIR variable. However, MSYS2 translates such Unix-style paths into actual Windows filesystem paths when it passes command-line arguments and environment variables to any programs it executes, including our Git LFS client. Thus the "git lfs env" command returns a Windows-style path for the GIT_LFS_TEST_DIR variable, while our Bash test scripts are expecting a Unix-style path beginning with /tmp, and so many of our tests in the t/t-env.sh and t/t-workflow.sh test scripts fail. To resolve this, when running on Windows, we set the MSYS2_ENV_CONV_EXCL environment variable at the top of these scripts to contain the name of the GIT_LFS_TEST_DIR variable. As described in the MSYS2 documentation, this ensures MSYS2 does not alter the value of the GIT_LFS_TEST_DIR variable: https://www.msys2.org/docs/filesystem-paths/#environment-variables Finally, we take the opportunity to ensure that the command substitutions we use to set the GIT_LFS_TEST_DIR environment variable are fully quoted, and to clean up some nearby excess whitespace.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 6, 2024
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepency is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 6, 2024
In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. The lfstest-count-tests utility did not work as intended on the Windows platform, however, as noted in the description of commit commit 9b73c80 in the same PR: On the AppVeyor CI Windows environment, there exist issues that prevent the lfstest-count-tests program from acquiring the test_count.lock file correctly. Rather than debug this problem, several workarounds were added, such as the GIT_LFS_NO_TEST_COUNT environment variable, which disables the use of the lfstest-count-tests utility in our individual t/t-*.sh test scripts, and the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, which is used to prevent the lfstest-count-tests utility from trying to acquire the test_count.lock file at all on Windows. The bug which causes the lfstest-count-tests program to be unable to acquire the lock file the second time it is run is the result of the lock file not being removed when the program runs the first time. This in turn is due to the fact that the file handle returned by the call to the OpenFile() function of the "os" package is never released with a call to Close(), so the subsequent call to Remove() results in failure. On Unix systems, this does not occur, because the file can be unlinked while file handles to it remain open. On Windows, however, all file handles must be closed before the file can be deleted. We fix the underlying bug here by simply calling Close() on the new file handle immediately after creating the file, so long as no error was returned by the call to OpenFile(). In addition, we make a number of other fixes and enhancements to the lfstest-count-tests program. We use the more conventional permissions mode of 0777 for any directories we create, rather than 0666. We include the O_RDWR flag with the others we pass to the OpenFile() function, since we are technically supposed to provide at least one of O_RDWR, O_RDONLY, and O_WRONLY. And we output an error message if the deferred call to our release() function is unable to delete the lock file.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 6, 2024
In commit 9b73c80 of PR git-lfs#3125 the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable was introduced for use with our test suites. When this variable was set to the value "1", it indicated to the lfstest-count-tests test utility that it should skip creating a test_count.lock file before updating the test_count file we use to track the number of active test suites. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem by skipping the creation of a lock file altogether on Windows. This change was also made in conjunction with the addition of another environment variable, GIT_LFS_NO_TEST_COUNT, in commit c591ff7 of the same PR. That variable is also set only on Windows, and stops the setup() and shutdown() shell functions defined in our t/testhelpers.sh script from calling the lfstest-count-tests program. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can now remove both of these environment variables and the test suite features they control. We start by removing support for the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable from the lfstest-count-tests utility, and changing our script/cibuild script so it no longer sets the variable. We can also remove the environment variable from the list of those we unset before running certain tests. We had to introduce this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 6, 2024
In commit c591ff7 of PR git-lfs#3125 the GIT_LFS_NO_TEST_COUNT environment variable was introduced for use with our test suites. When this variable was set to a non-empty value, it indicated to the setup() and shutdown() test helper shell functions that they should skip running the lfstest-count-tests utility program entirely. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem in part by not counting the number of running test scripts on Windows. (We also introduced the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, in commit 9b73c80 of the same PR, to further address the problem.) When running our test suite on Windows, we set the GIT_LFS_NO_TEST_COUNT to "1", which causes the individual test suites to skip invoking lfstest-count-tests entirely. Instead, the program is run just once in the "test" t/Makefile target before executing the test suites with the "prove" command, and then once again afterwards, both times by explicitly resetting the GIT_LFS_NO_TEST_COUNT variable to an empty string value. These two invocations cause the lfstest-count-tests program to start the lfstest-gitserver program before all the test suites were executed and then stop it once they had all finished. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can remove this environment variable and the test suite features it controls. We change our script/cibuild script to no longer set the GIT_LFS_NO_TEST_COUNT variable, and we revise the conditionals in the setup() and shutdown() test helper shell functions so they always call the lfstest-count-tests program, rather than only doing so if the GIT_LFS_NO_TEST_COUNT variable is unset or empty. We also change the "test" target's recipe in our t/Makefile file to not unset GIT_LFS_NO_TEST_COUNT when running lfstest-count-tests before and after it runs the "prove" command. Finally, as we have already removed the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable and all support for it, in a previous commit in this PR, that allows us now to also eliminate the unset_vars() shell functions in some of our t/t-*.sh test scripts, which was used to unset both that variable and the GIT_LFS_NO_TEST_COUNT variable before running the tests in those scripts. We added this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 6, 2024
In commit e1c2c8a of PR git-lfs#1107 we revised the test to use a separate test directory, either a temporary one we create or one specified in advance in a GIT_LFS_TEST_DIR environment variable. If that variable is empty or unset, we populate it with the output of a "mktemp -d" command with a directory name pattern of "git-lfs_TEMP". In either case, we then create all of our test repositories in subdirectories of the location given by GIT_LFS_TEST_DIR. If we create a directory because GIT_LFS_TEST_DIR is empty or unset, we also set the RM_GIT_LFS_TEST_DIR variable with a flag value of "yes". As of commit e1c2c8a, we would then check that variable in the shutdown() shell function, and if it was set to "yes", we would then remove the directory specified by the GIT_LFS_TEST_DIR variable if it matched the pattern we use for temporary directories and the KEEPTRASH variable was empty or unset. This ensured that, in the normal case, we removed all of our test artifacts after completing the test suite. In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. In that commmit, the "test" target of the t/Makefile file was written so it first loads the t/testenv.sh script and then runs the setup() shell function, which calls "lfstest-count-tests increment", which starts the lfstest-gitserver program because it increments the test count from zero to one. Every test script run by the "prove" command then calls setup() again, which further increments the test count, and then as it exits it calls the shutdown() shell function, which runs the "lfstest-count-tests decrement" command. That never decrements the test count below one, however, because of the initial call to setup() prior to running "prove". Finally, the "test" Makefile target runs shutdown() after "prove" is complete, which decrements the test count to zero, at which point lfstest-count-tests sends a shutdown POST HTTP request to the running lfstest-gitserver instance. In this design, though, each individual test script loads the t/testenv.sh script, and the script is directly loaded twice by the "test" Makefile target's recipe. In each case, it script runs in a separate shell session, and so does not find a GIT_LFS_TEST_DIR environment variable (unless one is set in the user's shell session when running "make test"). Thus each invocation of t/testenv.sh results in the creation of a new temporary test directory. Commit 1926e94 also removed the logic from the shutdown() shell function which handled the deletion of the directory specified by the GIT_LFS_TEST_DIR variable, if the RM_GIT_LFS_TEST_DIR variable was set to "yes" and we had clearly created a temporary directory to populate GIT_LFS_TEST_DIR. The consequence is that when we run an individual test script by itself, it leaves a single temporary test directory behind, and when we run the "make test" Makefile target, every t/t-*.sh script leaves a test directory behind, plus two additional ones created when the recipe loads the t/testenv.sh script directly. We can improve this situation by restoring the logic to clean up the directory specified by the GIT_LFS_TEST_DIR variable (if the RM_GIT_LFS_TEST_DIR variable contains "yes" and the other conditions described above are met), and then adjusting our "test" target's recipe so it runs all of its key steps in a single Bash shell session. We also make sure to export the GIT_LFS_TEST_DIR and RM_GIT_LFS_TEST_DIR variables into the environment, so that when t/testenv.sh is loaded by the recipe the first time, it creates a temporary directory and exports those variables, which ensures they are then available to all the subsequent test scripts and the recipe's final call to shutdown(). By making these changes, we ensure that in the normal case, when GIT_LFS_TEST_DIR is empty or unset and we run "make test", we create only a single temporary test directory, use it in all the individual test scripts, and then remove it completely afterwards. In our t/t-env.sh and t/t-workflow.sh test scripts, we check the output of the "git lfs env" command in a number of tests, comparing it to the values we expect based on the test conditions, including the values of all environment variables whose names begin with the "GIT_" prefix. As we now export the GIT_LFS_TEST_DIR variable into the environment, it is among these environment variables, which causes the tests to fail on Windows unless we make one additional change. We run our CI test suite on Windows using the Git Bash environment provided by the Git for Windows project, which is in turn based on the MSYS2 environment. In this context, the mktemp(1) command returns a path beginning with /tmp, so that is what is stored in our GIT_LFS_TEST_DIR variable. However, MSYS2 translates such Unix-style paths into actual Windows filesystem paths when it passes command-line arguments and environment variables to any programs it executes, including our Git LFS client. Thus the "git lfs env" command returns a Windows-style path for the GIT_LFS_TEST_DIR variable, while our Bash test scripts are expecting a Unix-style path beginning with /tmp, and so many of our tests in the t/t-env.sh and t/t-workflow.sh test scripts fail. To resolve this, when running on Windows, we set the MSYS2_ENV_CONV_EXCL environment variable at the top of these scripts to contain the name of the GIT_LFS_TEST_DIR variable. As described in the MSYS2 documentation, this ensures MSYS2 does not alter the value of the GIT_LFS_TEST_DIR variable: https://www.msys2.org/docs/filesystem-paths/#environment-variables Finally, we take the opportunity to ensure that the command substitutions we use to set the GIT_LFS_TEST_DIR environment variable are fully quoted, and to clean up some nearby excess whitespace.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 8, 2024
Since commit c591ff7 of PR git-lfs#3125 we have installed Strawberry Perl before running our CI test suite on Windows. This was necessary when we used AppVeyor to run our test suite on Windows, and subsequently also when we converted our CI jobs to GitHub Actions in PR git-lfs#3808. We require Perl because we use the "prove" command, which runs all of our t/t-*.sh test scripts and collects and summarizes the results. However, since commit 8818630 of PR git-lfs#5666 we have installed the Git for Windows SDK before running our test suite, and it includes a full Perl distribution, which means we no longer need to install Strawberry Perl separately.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 8, 2024
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepency is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 8, 2024
In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. The lfstest-count-tests utility did not work as intended on the Windows platform, however, as noted in the description of commit commit 9b73c80 in the same PR: On the AppVeyor CI Windows environment, there exist issues that prevent the lfstest-count-tests program from acquiring the test_count.lock file correctly. Rather than debug this problem, several workarounds were added, such as the GIT_LFS_NO_TEST_COUNT environment variable, which disables the use of the lfstest-count-tests utility in our individual t/t-*.sh test scripts, and the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, which is used to prevent the lfstest-count-tests utility from trying to acquire the test_count.lock file at all on Windows. The bug which causes the lfstest-count-tests program to be unable to acquire the lock file the second time it is run is the result of the lock file not being removed when the program runs the first time. This in turn is due to the fact that the file handle returned by the call to the OpenFile() function of the "os" package is never released with a call to Close(), so the subsequent call to Remove() results in failure. On Unix systems, this does not occur, because the file can be unlinked while file handles to it remain open. On Windows, however, all file handles must be closed before the file can be deleted. We fix the underlying bug here by simply calling Close() on the new file handle immediately after creating the file, so long as no error was returned by the call to OpenFile(). In addition, we make a number of other fixes and enhancements to the lfstest-count-tests program. We use the more conventional permissions mode of 0777 for any directories we create, rather than 0666. We include the O_RDWR flag with the others we pass to the OpenFile() function, since we are technically supposed to provide at least one of O_RDWR, O_RDONLY, and O_WRONLY. And we output an error message if the deferred call to our release() function is unable to delete the lock file.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 8, 2024
In commit 9b73c80 of PR git-lfs#3125 the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable was introduced for use with our test suites. When this variable was set to the value "1", it indicated to the lfstest-count-tests test utility that it should skip creating a test_count.lock file before updating the test_count file we use to track the number of active test suites. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem by skipping the creation of a lock file altogether on Windows. This change was also made in conjunction with the addition of another environment variable, GIT_LFS_NO_TEST_COUNT, in commit c591ff7 of the same PR. That variable is also set only on Windows, and stops the setup() and shutdown() shell functions defined in our t/testhelpers.sh script from calling the lfstest-count-tests program. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can now remove both of these environment variables and the test suite features they control. We start by removing support for the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable from the lfstest-count-tests utility, and changing our script/cibuild script so it no longer sets the variable. We can also remove the environment variable from the list of those we unset before running certain tests. We had to introduce this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 8, 2024
In commit c591ff7 of PR git-lfs#3125 the GIT_LFS_NO_TEST_COUNT environment variable was introduced for use with our test suites. When this variable was set to a non-empty value, it indicated to the setup() and shutdown() test helper shell functions that they should skip running the lfstest-count-tests utility program entirely. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem in part by not counting the number of running test scripts on Windows. (We also introduced the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, in commit 9b73c80 of the same PR, to further address the problem.) When running our test suite on Windows, we set the GIT_LFS_NO_TEST_COUNT to "1", which causes the individual test suites to skip invoking lfstest-count-tests entirely. Instead, the program is run just once in the "test" t/Makefile target before executing the test suites with the "prove" command, and then once again afterwards, both times by explicitly resetting the GIT_LFS_NO_TEST_COUNT variable to an empty string value. These two invocations cause the lfstest-count-tests program to start the lfstest-gitserver program before all the test suites were executed and then stop it once they had all finished. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can remove this environment variable and the test suite features it controls. We change our script/cibuild script to no longer set the GIT_LFS_NO_TEST_COUNT variable, and we revise the conditionals in the setup() and shutdown() test helper shell functions so they always call the lfstest-count-tests program, rather than only doing so if the GIT_LFS_NO_TEST_COUNT variable is unset or empty. We also change the "test" target's recipe in our t/Makefile file to not unset GIT_LFS_NO_TEST_COUNT when running lfstest-count-tests before and after it runs the "prove" command. Finally, as we have already removed the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable and all support for it, in a previous commit in this PR, that allows us now to also eliminate the unset_vars() shell functions in some of our t/t-*.sh test scripts, which was used to unset both that variable and the GIT_LFS_NO_TEST_COUNT variable before running the tests in those scripts. We added this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 8, 2024
In commit e1c2c8a of PR git-lfs#1107 we revised the test to use a separate test directory, either a temporary one we create or one specified in advance in a GIT_LFS_TEST_DIR environment variable. If that variable is empty or unset, we populate it with the output of a "mktemp -d" command with a directory name pattern of "git-lfs_TEMP". In either case, we then create all of our test repositories in subdirectories of the location given by GIT_LFS_TEST_DIR. If we create a directory because GIT_LFS_TEST_DIR is empty or unset, we also set the RM_GIT_LFS_TEST_DIR variable with a flag value of "yes". As of commit e1c2c8a, we would then check that variable in the shutdown() shell function, and if it was set to "yes", we would then remove the directory specified by the GIT_LFS_TEST_DIR variable if it matched the pattern we use for temporary directories and the KEEPTRASH variable was empty or unset. This ensured that, in the normal case, we removed all of our test artifacts after completing the test suite. In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. In that commmit, the "test" target of the t/Makefile file was written so it first loads the t/testenv.sh script and then runs the setup() shell function, which calls "lfstest-count-tests increment", which starts the lfstest-gitserver program because it increments the test count from zero to one. Every test script run by the "prove" command then calls setup() again, which further increments the test count, and then as it exits it calls the shutdown() shell function, which runs the "lfstest-count-tests decrement" command. That never decrements the test count below one, however, because of the initial call to setup() prior to running "prove". Finally, the "test" Makefile target runs shutdown() after "prove" is complete, which decrements the test count to zero, at which point lfstest-count-tests sends a shutdown POST HTTP request to the running lfstest-gitserver instance. In this design, though, each individual test script loads the t/testenv.sh script, and the script is directly loaded twice by the "test" Makefile target's recipe. In each case, it script runs in a separate shell session, and so does not find a GIT_LFS_TEST_DIR environment variable (unless one is set in the user's shell session when running "make test"). Thus each invocation of t/testenv.sh results in the creation of a new temporary test directory. Commit 1926e94 also removed the logic from the shutdown() shell function which handled the deletion of the directory specified by the GIT_LFS_TEST_DIR variable, if the RM_GIT_LFS_TEST_DIR variable was set to "yes" and we had clearly created a temporary directory to populate GIT_LFS_TEST_DIR. The consequence is that when we run an individual test script by itself, it leaves a single temporary test directory behind, and when we run the "make test" Makefile target, every t/t-*.sh script leaves a test directory behind, plus two additional ones created when the recipe loads the t/testenv.sh script directly. We can improve this situation by restoring the logic to clean up the directory specified by the GIT_LFS_TEST_DIR variable (if the RM_GIT_LFS_TEST_DIR variable contains "yes" and the other conditions described above are met), and then adjusting our "test" target's recipe so it runs all of its key steps in a single Bash shell session. We also make sure to export the GIT_LFS_TEST_DIR and RM_GIT_LFS_TEST_DIR variables into the environment, so that when t/testenv.sh is loaded by the recipe the first time, it creates a temporary directory and exports those variables, which ensures they are then available to all the subsequent test scripts and the recipe's final call to shutdown(). By making these changes, we ensure that in the normal case, when GIT_LFS_TEST_DIR is empty or unset and we run "make test", we create only a single temporary test directory, use it in all the individual test scripts, and then remove it completely afterwards. In our t/t-env.sh and t/t-workflow.sh test scripts, we check the output of the "git lfs env" command in a number of tests, comparing it to the values we expect based on the test conditions, including the values of all environment variables whose names begin with the "GIT_" prefix. As we now export the GIT_LFS_TEST_DIR variable into the environment, it is among these environment variables, which causes the tests to fail on Windows unless we make one additional change. We run our CI test suite on Windows using the Git Bash environment provided by the Git for Windows project, which is in turn based on the MSYS2 environment. In this context, the mktemp(1) command returns a path beginning with /tmp, so that is what is stored in our GIT_LFS_TEST_DIR variable. However, MSYS2 translates such Unix-style paths into actual Windows filesystem paths when it passes command-line arguments and environment variables to any programs it executes, including our Git LFS client. Thus the "git lfs env" command returns a Windows-style path for the GIT_LFS_TEST_DIR variable, while our Bash test scripts are expecting a Unix-style path beginning with /tmp, and so many of our tests in the t/t-env.sh and t/t-workflow.sh test scripts fail. To resolve this, when running on Windows, we set the MSYS2_ENV_CONV_EXCL environment variable at the top of these scripts to contain the name of the GIT_LFS_TEST_DIR variable. As described in the MSYS2 documentation, this ensures MSYS2 does not alter the value of the GIT_LFS_TEST_DIR variable: https://www.msys2.org/docs/filesystem-paths/#environment-variables Finally, we take the opportunity to ensure that the command substitutions we use to set the GIT_LFS_TEST_DIR environment variable are fully quoted, and to clean up some nearby excess whitespace.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
The SHUTDOWN_LFS environment variable was added as part of the original integration test suite in commit b1f5025 of PR git-lfs#306, and was used to indicate whether the Git server run as part of the test suite should be stopped when the shutdown() shell function ran at the end of a set of tests, or only when the last set of tests completed. When the integration test suite was refactored to use the "prove" command and to align with Git's test suite, in PR git-lfs#3125, most references to the SHUTDOWN_LFS environment variable were removed, particularly in commits 1926e94 and a4cc106. However, one reference was left in the t/testlib.sh script, so we remove it now.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
Since commit c591ff7 of PR git-lfs#3125 we have installed Strawberry Perl before running our CI test suite on Windows. This was necessary when we used AppVeyor to run our test suite on Windows, and subsequently also when we converted our CI jobs to GitHub Actions in PR git-lfs#3808. We require Perl because we use the "prove" command, which runs all of our t/t-*.sh test scripts and collects and summarizes the results. However, since commit 8818630 of PR git-lfs#5666 we have installed the Git for Windows SDK before running our test suite, and it includes a full Perl distribution, which means we no longer need to install Strawberry Perl separately.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepency is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. The lfstest-count-tests utility did not work as intended on the Windows platform, however, as noted in the description of commit commit 9b73c80 in the same PR: On the AppVeyor CI Windows environment, there exist issues that prevent the lfstest-count-tests program from acquiring the test_count.lock file correctly. Rather than debug this problem, several workarounds were added, such as the GIT_LFS_NO_TEST_COUNT environment variable, which disables the use of the lfstest-count-tests utility in our individual t/t-*.sh test scripts, and the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, which is used to prevent the lfstest-count-tests utility from trying to acquire the test_count.lock file at all on Windows. The bug which causes the lfstest-count-tests program to be unable to acquire the lock file the second time it is run is the result of the lock file not being removed when the program runs the first time. This in turn is due to the fact that the file handle returned by the call to the OpenFile() function of the "os" package is never released with a call to Close(), so the subsequent call to Remove() results in failure. On Unix systems, this does not occur, because the file can be unlinked while file handles to it remain open. On Windows, however, all file handles must be closed before the file can be deleted. We fix the underlying bug here by simply calling Close() on the new file handle immediately after creating the file, so long as no error was returned by the call to OpenFile(). In addition, we make a number of other fixes and enhancements to the lfstest-count-tests program. We use the more conventional permissions mode of 0777 for any directories we create, rather than 0666. We include the O_RDWR flag with the others we pass to the OpenFile() function, since we are technically supposed to provide at least one of O_RDWR, O_RDONLY, and O_WRONLY. And we output an error message if the deferred call to our release() function is unable to delete the lock file.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
In commit 9b73c80 of PR git-lfs#3125 the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable was introduced for use with our test suites. When this variable was set to the value "1", it indicated to the lfstest-count-tests test utility that it should skip creating a test_count.lock file before updating the test_count file we use to track the number of active test suites. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem by skipping the creation of a lock file altogether on Windows. This change was also made in conjunction with the addition of another environment variable, GIT_LFS_NO_TEST_COUNT, in commit c591ff7 of the same PR. That variable is also set only on Windows, and stops the setup() and shutdown() shell functions defined in our t/testhelpers.sh script from calling the lfstest-count-tests program. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can now remove both of these environment variables and the test suite features they control. We start by removing support for the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable from the lfstest-count-tests utility, and changing our script/cibuild script so it no longer sets the variable. We can also remove the environment variable from the list of those we unset before running certain tests. We had to introduce this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
In commit c591ff7 of PR git-lfs#3125 the GIT_LFS_NO_TEST_COUNT environment variable was introduced for use with our test suites. When this variable was set to a non-empty value, it indicated to the setup() and shutdown() test helper shell functions that they should skip running the lfstest-count-tests utility program entirely. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem in part by not counting the number of running test scripts on Windows. (We also introduced the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, in commit 9b73c80 of the same PR, to further address the problem.) When running our test suite on Windows, we set the GIT_LFS_NO_TEST_COUNT to "1", which causes the individual test suites to skip invoking lfstest-count-tests entirely. Instead, the program is run just once in the "test" t/Makefile target before executing the test suites with the "prove" command, and then once again afterwards, both times by explicitly resetting the GIT_LFS_NO_TEST_COUNT variable to an empty string value. These two invocations cause the lfstest-count-tests program to start the lfstest-gitserver program before all the test suites were executed and then stop it once they had all finished. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can remove this environment variable and the test suite features it controls. We change our script/cibuild script to no longer set the GIT_LFS_NO_TEST_COUNT variable, and we revise the conditionals in the setup() and shutdown() test helper shell functions so they always call the lfstest-count-tests program, rather than only doing so if the GIT_LFS_NO_TEST_COUNT variable is unset or empty. We also change the "test" target's recipe in our t/Makefile file to not unset GIT_LFS_NO_TEST_COUNT when running lfstest-count-tests before and after it runs the "prove" command. Finally, as we have already removed the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable and all support for it, in a previous commit in this PR, that allows us now to also eliminate the unset_vars() shell functions in some of our t/t-*.sh test scripts, which was used to unset both that variable and the GIT_LFS_NO_TEST_COUNT variable before running the tests in those scripts. We added this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI test suite to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
In commit e1c2c8a of PR git-lfs#1107 we revised the test to use a separate test directory, either a temporary one we create or one specified in advance in a GIT_LFS_TEST_DIR environment variable. If that variable is empty or unset, we populate it with the output of a "mktemp -d" command with a directory name pattern of "git-lfs_TEMP". In either case, we then create all of our test repositories in subdirectories of the location given by GIT_LFS_TEST_DIR. If we create a directory because GIT_LFS_TEST_DIR is empty or unset, we also set the RM_GIT_LFS_TEST_DIR variable with a flag value of "yes". As of commit e1c2c8a, we would then check that variable in the shutdown() shell function, and if it was set to "yes", we would then remove the directory specified by the GIT_LFS_TEST_DIR variable if it matched the pattern we use for temporary directories and the KEEPTRASH variable was empty or unset. This ensured that, in the normal case, we removed all of our test artifacts after completing the test suite. In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. In that commmit, the "test" target of the t/Makefile file was written so it first loads the t/testenv.sh script and then runs the setup() shell function, which calls "lfstest-count-tests increment", which starts the lfstest-gitserver program because it increments the test count from zero to one. Every test script run by the "prove" command then calls setup() again, which further increments the test count, and then as it exits it calls the shutdown() shell function, which runs the "lfstest-count-tests decrement" command. That never decrements the test count below one, however, because of the initial call to setup() prior to running "prove". Finally, the "test" Makefile target runs shutdown() after "prove" is complete, which decrements the test count to zero, at which point lfstest-count-tests sends a shutdown POST HTTP request to the running lfstest-gitserver instance. In this design, though, each individual test script loads the t/testenv.sh script, and the script is directly loaded twice by the "test" Makefile target's recipe. In each case, it script runs in a separate shell session, and so does not find a GIT_LFS_TEST_DIR environment variable (unless one is set in the user's shell session when running "make test"). Thus each invocation of t/testenv.sh results in the creation of a new temporary test directory. Commit 1926e94 also removed the logic from the shutdown() shell function which handled the deletion of the directory specified by the GIT_LFS_TEST_DIR variable, if the RM_GIT_LFS_TEST_DIR variable was set to "yes" and we had clearly created a temporary directory to populate GIT_LFS_TEST_DIR. The consequence is that when we run an individual test script by itself, it leaves a single temporary test directory behind, and when we run the "make test" Makefile target, every t/t-*.sh script leaves a test directory behind, plus two additional ones created when the recipe loads the t/testenv.sh script directly. We can improve this situation by restoring the logic to clean up the directory specified by the GIT_LFS_TEST_DIR variable (if the RM_GIT_LFS_TEST_DIR variable contains "yes" and the other conditions described above are met), and then adjusting our "test" target's recipe so it runs all of its key steps in a single Bash shell session. We also make sure to export the GIT_LFS_TEST_DIR and RM_GIT_LFS_TEST_DIR variables into the environment, so that when t/testenv.sh is loaded by the recipe the first time, it creates a temporary directory and exports those variables, which ensures they are then available to all the subsequent test scripts and the recipe's final call to shutdown(). By making these changes, we ensure that in the normal case, when GIT_LFS_TEST_DIR is empty or unset and we run "make test", we create only a single temporary test directory, use it in all the individual test scripts, and then remove it completely afterwards. In our t/t-env.sh and t/t-workflow.sh test scripts, we check the output of the "git lfs env" command in a number of tests, comparing it to the values we expect based on the test conditions, including the values of all environment variables whose names begin with the "GIT_" prefix. As we now export the GIT_LFS_TEST_DIR variable into the environment, it is among these environment variables, which causes the tests to fail on Windows unless we make one additional change. We run our CI test suite on Windows using the Git Bash environment provided by the Git for Windows project, which is in turn based on the MSYS2 environment. In this context, the mktemp(1) command returns a path beginning with /tmp, so that is what is stored in our GIT_LFS_TEST_DIR variable. However, MSYS2 translates such Unix-style paths into actual Windows filesystem paths when it passes command-line arguments and environment variables to any programs it executes, including our Git LFS client. Thus the "git lfs env" command returns a Windows-style path for the GIT_LFS_TEST_DIR variable, while our Bash test scripts are expecting a Unix-style path beginning with /tmp, and so many of our tests in the t/t-env.sh and t/t-workflow.sh test scripts fail. To resolve this, when running on Windows, we set the MSYS2_ENV_CONV_EXCL environment variable at the top of these scripts to contain the name of the GIT_LFS_TEST_DIR variable. As described in the MSYS2 documentation, this ensures MSYS2 does not alter the value of the GIT_LFS_TEST_DIR variable: https://www.msys2.org/docs/filesystem-paths/#environment-variables Finally, we take the opportunity to ensure that the command substitutions we use to set the GIT_LFS_TEST_DIR environment variable are fully quoted, and to clean up some nearby excess whitespace.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
The SHUTDOWN_LFS environment variable was added as part of the original integration test suite implementation in commit b1f5025 of PR git-lfs#306, and was used to indicate whether the Git server which runs as part of the test suite should be stopped by the shutdown() shell function at the end of a set of tests, or only when the last set of tests completed. When the integration test suite was refactored to use the "prove" command and to align with Git's test suite, in PR git-lfs#3125, most references to the SHUTDOWN_LFS environment variable were removed, particularly in commits 1926e94 and a4cc106. However, one reference was left in the t/testlib.sh script, so we remove it now.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
Since commit c591ff7 of PR git-lfs#3125 we have installed Strawberry Perl before running our CI test suite on Windows. This was necessary when we used the AppVeyor service to run our test suite on Windows, and subsequently also when we converted our CI jobs to GitHub Actions in PR git-lfs#3808. We require Perl because we use the "prove" command, which runs all of our t/t-*.sh test scripts and collects and summarizes the results. However, since commit 8818630 of PR git-lfs#5666 we have installed the Git for Windows SDK before running our test suite, and it includes a full Perl distribution, which means we no longer need to install Strawberry Perl separately.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
Since commit 9b0adeb of PR git-lfs#3125, when we converted our test suite to use the "prove" command, we have run up to nine test jobs in parallel in our primary test builds. However, as of commit 7fd5aa6 in PR git-lfs#3144, we only use a maximum of four parallel test jobs when building our Linux RPM packages. This has the effect of slightly slowing down our CI suite, because, ever since commit 212c051 in PR git-lfs#3932, we build a range of Linux packages in our GitHub Actions CI workflow. (Note that for historical reasons, we only run the full "make test" Makefile target in our "t" test directory when building RPM packages; in our Debian packages, we just run the Go language tests. While this discrepancy is worth addressing, for the moment we simply aim to improve the runtime of our RPM package builds.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
In commit 1926e94 of PR git-lfs#3125 we migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. The lfstest-count-tests utility did not work as intended on the Windows platform, however, as noted in the description of commit 9b73c80 in the same PR: On the AppVeyor CI Windows environment, there exist issues that prevent the lfstest-count-tests program from acquiring the test_count.lock file correctly. Rather than debug this problem, several workarounds were added, such as the GIT_LFS_NO_TEST_COUNT environment variable, which disables the use of the lfstest-count-tests utility in our individual t/t-*.sh test scripts, and the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, which is used to prevent the lfstest-count-tests utility from trying to acquire the test_count.lock file at all on Windows. The bug which causes the lfstest-count-tests program to be unable to acquire the lock file the second time the program is run is the result of the lock file not being removed when the program runs the first time. This in turn is due to the fact that the file handle returned by the call to the OpenFile() function of the "os" package is never released with a call to Close(), so the subsequent call to Remove() results in failure. On Unix systems, this does not occur, because the file can be unlinked while file handles to it remain open. On Windows, however, all file handles must be closed before the file can be deleted. We fix the underlying bug here by simply calling Close() on the new file handle immediately after creating the file, so long as no error was returned by the call to OpenFile(). In addition, we make a number of other fixes and enhancements to the lfstest-count-tests program. We use the more conventional permissions mode of 0777 for any directories we create, rather than 0666. We include the O_RDWR flag with the others we pass to the OpenFile() function, since we are technically supposed to provide at least one of O_RDWR, O_RDONLY, and O_WRONLY. And we output an error message if the deferred call to our release() function is unable to delete the lock file.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
In commit 9b73c80 of PR git-lfs#3125 the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable was introduced for use with our test suite. When this variable is set to the value "1", it indicates to the lfstest-count-tests test utility that it should skip creating a test_count.lock file before updating the test_count file we use to track the number of active test scripts. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file on Windows, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem by skipping the creation of a lock file altogether on Windows. This change was also made in conjunction with the addition of another environment variable, GIT_LFS_NO_TEST_COUNT, in commit c591ff7 of the same PR. That variable is also set only on Windows, and stops the setup() and shutdown() shell functions defined in our t/testhelpers.sh script from calling the lfstest-count-tests program. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can now remove both of these environment variables and the test suite features they control. We start by removing support for the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable from the lfstest-count-tests utility, and changing our script/cibuild script so it no longer sets the variable. We can also remove the environment variable from the list of those we unset before running certain tests. We had to introduce this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI jobs to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they were set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
In commit c591ff7 of PR git-lfs#3125 the GIT_LFS_NO_TEST_COUNT environment variable was introduced for use with our test suite. When this variable is set to a non-empty value, it indicates to the setup() and shutdown() test helper shell functions that they should skip running the lfstest-count-tests utility program entirely. This was done because the lfstest-count-tests program had a bug which prevented it from deleting the lock file on Windows, causing subsequent invocations of the program to be unable to exclusively create it again. As this bug was not resolved at the time, the choice was made to work around the problem in part by not counting the number of running test scripts on Windows. (We also introduced the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable, in commit 9b73c80 of the same PR, to further address the problem.) When running our test suite on Windows, we set the GIT_LFS_NO_TEST_COUNT to "1", which causes the individual test scripts to skip invoking lfstest-count-tests entirely. Instead, the program is run just once in the "test" t/Makefile target before executing the test scripts with the "prove" command, and then once again afterwards, both times by explicitly resetting the GIT_LFS_NO_TEST_COUNT variable to an empty string value. These two invocations cause the lfstest-count-tests program to start the lfstest-gitserver program before all the test scripts are executed and then stop it once they have all finished. As we have now resolved the underlying problem in the lfstest-count-tests utility, in a prior commit in this PR, we can remove this environment variable and the test suite features it controls. We change our script/cibuild script to no longer set the GIT_LFS_NO_TEST_COUNT variable, and we revise the conditionals in the setup() and shutdown() test helper shell functions so they always call the lfstest-count-tests program, rather than only doing so if the GIT_LFS_NO_TEST_COUNT variable is unset or empty. We also change the "test" target's recipe in our t/Makefile file to not unset GIT_LFS_NO_TEST_COUNT when running lfstest-count-tests before and after it runs the "prove" command. Finally, as we have already removed the GIT_LFS_LOCK_ACQUIRE_DISABLED environment variable and all support for it, in a previous commit in this PR, we are now also able to eliminate the unset_vars() shell functions in some of our t/t-*.sh test scripts. These functions are used to unset both the GIT_LFS_LOCK_ACQUIRE_DISABLED and GIT_LFS_NO_TEST_COUNT variables before running the tests in those scripts. We added this behaviour in commit aca1aff of PR git-lfs#3808, when we migrated our CI jobs to GitHub Actions. However, we will no longer need to unset these variables as we will now never set them at all, and they will have no effect if they are set.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 5, 2024
In commit e1c2c8a of PR git-lfs#1107 we revised our test suite to use a separate test directory, either a temporary one we create or one specified in advance in a GIT_LFS_TEST_DIR environment variable. If that variable is empty or unset, we populate it with the output of a "mktemp -d" command with a directory name prefix pattern of "git-lfs_TEMP". We then create all of our test repositories in subdirectories of the location given by GIT_LFS_TEST_DIR. If we create a directory because GIT_LFS_TEST_DIR is empty or unset, we also set the RM_GIT_LFS_TEST_DIR variable with a flag value of "yes". As of commit e1c2c8a, we would then check that variable in the shutdown() shell function, and if the variable was set to "yes", we would then remove the directory specified by the GIT_LFS_TEST_DIR variable if it matched the pattern we use for temporary directories and if the KEEPTRASH variable was empty or unset. This ensured that, in the normal case, we removed all of our test artifacts after completing our test scripts. In commit 1926e94 of PR git-lfs#3125 we then migrated our test suite to use the "prove" command, and introduced the lfstest-count-tests utility to help control a common instance of our lfstest-gitserver program, which can be shared and reused by all the individual test scripts. In that PR we also moved our test suite into the "t" directory, and added a Makefile with a "test" target that runs the "prove" command over all the individual test scripts; this is still the framework we use today. In that commit, the "test" target's recipe in the t/Makefile file was written so it first loads the t/testenv.sh script and then runs the setup() shell function, which calls "lfstest-count-tests increment", which starts the lfstest-gitserver program because it increments the test count from zero to one. Every test script run by the "prove" command then calls setup() again, which further increments the test count, and then as it exits it calls the shutdown() shell function, which runs the "lfstest-count-tests decrement" command. That never decrements the test count below one, however, because of the initial call to setup() made prior to running "prove". Finally, the "test" Makefile target recpie runs shutdown() after "prove" is complete, which decrements the test count to zero, at which point lfstest-count-tests sends a shutdown POST HTTP request to the running lfstest-gitserver instance. In this design, though, each individual test script loads the t/testenv.sh script, and the script is directly loaded twice by the "test" Makefile target's recipe. In each case, the script runs in a separate shell session, and so does not find a GIT_LFS_TEST_DIR environment variable (unless one is set in the user's shell session when running "make test"). Thus each invocation of t/testenv.sh results in the creation of a new temporary test directory. Commit 1926e94 also removed the logic from the shutdown() shell function that handled the deletion of the directory specified by the GIT_LFS_TEST_DIR variable. The consequence is that when we run an individual test script by itself, it leaves a single temporary test directory behind, and when we run the "make test" Makefile target, every t/t-*.sh script leaves a test directory behind, plus two additional ones created when the recipe loads the t/testenv.sh script directly. We can improve this situation by restoring the logic to clean up the directory specified by the GIT_LFS_TEST_DIR variable (if the RM_GIT_LFS_TEST_DIR variable contains "yes" and the other conditions described above are met), and then adjusting our "test" target's recipe so it runs all of its key steps in a single Bash shell session. We also make sure to export the GIT_LFS_TEST_DIR and RM_GIT_LFS_TEST_DIR variables into the environment, so that when t/testenv.sh is loaded by the recipe the first time, it creates a temporary directory and exports those variables, which ensures they are then available to all the subsequent test scripts and the recipe's final call to shutdown(). By making these changes, we ensure that in the normal case, when GIT_LFS_TEST_DIR is empty or unset and we run "make test", we create only a single temporary test directory, use it in all the individual test scripts, and then remove it completely afterwards. In our t/t-env.sh and t/t-workflow.sh test scripts, we check the output of the "git lfs env" command in a number of tests, comparing it to the values we expect based on the test conditions, including the values of all environment variables whose names begin with the "GIT_" prefix. As we now export the GIT_LFS_TEST_DIR variable into the environment, it is among these environment variables, which causes the tests to fail on Windows unless we make one additional change. In our CI jobs, we run our test suite on Windows using the Git Bash environment provided by the Git for Windows project, which is in turn based on the MSYS2 environment. In this context, the mktemp(1) command returns a path beginning with /tmp, so that is now what is stored in our GIT_LFS_TEST_DIR environment variable. However, MSYS2 translates such Unix-style paths into actual Windows filesystem paths when it passes command-line arguments and environment variables to any programs it executes, including our Git LFS client. Thus the "git lfs env" command returns a Windows-style path for the GIT_LFS_TEST_DIR variable, while our tests in our t/t-env.sh and t/t-workflow.sh test scripts are expecting a Unix-style path beginning with /tmp. To resolve this, when running on Windows, we set the MSYS2_ENV_CONV_EXCL environment variable at the top of these scripts to contain the name of the GIT_LFS_TEST_DIR variable. As described in the MSYS2 documentation, this ensures MSYS2 does not alter the value of the GIT_LFS_TEST_DIR environment variable: https://www.msys2.org/docs/filesystem-paths/#environment-variables Finally, we take the opportunity to ensure that the command substitutions we use to set the GIT_LFS_TEST_DIR environment variable are fully quoted, and to clean up some nearby excess whitespace.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request converts the test output in
t(the directory formerly known astest) to the TAP specification, and replacesscript/integrationwitht/Makefilethat runsprove(1).We would like to move away from our home-grown integration scripts and towards something that more closely approximates the Git development environment, which should make it easier for contributors who are new to the Git LFS project (but perhaps familiar with the Git project) to get started.
This change does just that, by introducing support for the TAP specification and prove to run our integration tests. The changes included in this pull request are numerous, but following is a summary of the large themes:
Rename directory
testtot.proveexpects test files that match the shell globt/*.t.To approximate this, we introduce the
tdirectory (to replacetest), and prefix all files in that directory witht-. This is similar to git, which has tests such ast/t7810-grep.sh. Since our tests do not have numbering, this change replaces a file such astest/test-migrate-import.shwitht/t-migrate-import.sh.Replace
script/integration(andscript/integration.go) withmake test.Instead of continuing to perpetuate our home-grown
script/integration.go, let's replace it withprove(1), a widely-available test harness that understands TAP output and test parallelism instead. We can conveniently introduce aMakefileat the same time, which is useful for only rebuilding things that have changed since the last test invocation.Introduce
t/cmd/lfstest-count-tests.go.Previously,
test/testhelpers.shwas responsible for invokingsetup, a shell function that started a singlelfstest-git-server, which is a Git server used during integration tests. This works sufficiently well when one test is running at a time, but is prone to race conditions when two tests overlap one another in runtime. (For example, if test A starts before B (and therefore, launches a Git server) B will not launch a Git server because one has already been started by A. If A finishes before B, the Git server will go away before B has finished testing, and thus cause B to fail).Instead, we now maintain two files
test_count, andtest_count.lock, which synchronize the number of actively running tests. When the test_count goes from0to1, thelfstest-count-testsbinary starts a Git server running in the background. When the test_count goes from1back to0, the same binary stops that server as appropriate.To save time (and avoid a flapping git server), we implicitly set the count to
1when running more than one test in sequence, and clean up after ourselves later.For more: see t/README.md.
/cc @git-lfs/core