Skip to content

Conversation

@ttaylorr
Copy link
Contributor

This pull request converts the test output in t (the directory formerly known as test) to the TAP specification, and replaces script/integration with t/Makefile that runs prove(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 test to t. prove expects test files that match the shell glob t/*.t.

    To approximate this, we introduce the t directory (to replace test), and prefix all files in that directory with t-. This is similar to git, which has tests such as t/t7810-grep.sh. Since our tests do not have numbering, this change replaces a file such as test/test-migrate-import.sh with t/t-migrate-import.sh.

  • Replace script/integration (and script/integration.go) with make test.

    Instead of continuing to perpetuate our home-grown script/integration.go, let's replace it with prove(1), a widely-available test harness that understands TAP output and test parallelism instead. We can conveniently introduce a Makefile at 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.sh was responsible for invoking setup, a shell function that started a single lfstest-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, and test_count.lock, which synchronize the number of actively running tests. When the test_count goes from 0 to 1, the lfstest-count-tests binary starts a Git server running in the background. When the test_count goes from 1 back to 0, the same binary stops that server as appropriate.

    To save time (and avoid a flapping git server), we implicitly set the count to 1 when running more than one test in sequence, and clean up after ourselves later.

For more: see t/README.md.

/cc @git-lfs/core

ttaylorr added 30 commits July 9, 2018 16:07
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants