Integration tests 3 (one more time with feeling) #306

technoweenie · 2015-05-14T18:03:54Z

Code spike to see if #291 would work better with shell scripts. The main rationale is that these aren't Go tests, so running them with the "testing" package isn't necessary. Some of the tests will rely heavily on os.Chdir(), which affects the entire process. Even with goroutines, there's no guarantee that parallel tests won't run in the same process. Just running #291 with any kind of concurrency leads to weird errors.

Hopefully we can get around cross platform shell issues by using the common commands supported everywhere, and implement others in tiny Go commands. For instance, a command like lfstest-gitserver could spin up an HTTP server that responds to Git and Git LFS calls that tests use.

ignore the new tests subdir

michael-k · 2015-05-19T10:51:06Z

test/cmd/lfstest-gitserver.go

I'd prefer

by, ok := largeObjects[oid] if !ok { w.WriteHeader(404) return }

Good call. I like early returns like that.

technoweenie · 2015-05-19T21:20:57Z

This has progressed from a code spike to a PR that's ready for a review. I want to focus on the core test suite with the included "happy path" test as an example. After this is merged, I'll work on porting existing tests from the commands package to this new format.

While I'm waiting for all of the glorious reviews to come in, I'm going to work on a test/README.md doc that explains how tests work at a high level.

@michael-k: I made all of your suggested improvements.

michael-k · 2015-05-21T12:24:19Z

(…) test/remote directory (and the test git server that's booted up to expose it over HTTP) is "global" to all tests.

Yeah, I read about that in the README.md. 👍

The files are in .gitignore, so I don't know of any issues with leaving them there.

No issues. But it would be nice to have some script (e.g. script/clean) to clean them up. There might be more of them in the future and in different places. I'd say it's common practice for Makefiles to have a cleanup target, so it should be for tests too.

They could come in handy if you're troubleshooting some weird test failure.

Good point.

I may just clean them up now until it's proven that we should keep them around.

Maybe add an command line option to leave them behind?

technoweenie · 2015-05-21T16:34:05Z

@michael-k: I've updated the suite to remove the files unless $KEEPTRASH is set.

technoweenie · 2015-05-21T17:15:32Z

Unfortunately, I had to bring the explicit credential helper back due to unreliable CI builds:

I can retry them and get the failed ones to eventually pass, and vice versa. On a whim, I pushed 3bdfa6c to separate branch, and got 5 passes in a row. I'm merging this now. Though there is an opportunity for someone to either write a smaller test credential helper in shell, or figure out why the inline helper was failing sometimes.

Integration tests 3 (one more time with feeling)

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.

In commit 8e6641b of PR git-lfs#5803 we refactored our git-credential-lfstest test utility program so as to use a new internal "credential" structure to store the details of a given credential, as part of that PR's changes to support multi-stage authentication in a manner similar to that introduced in Git version 2.46.0. The git-credential-lfstest program acts as a Git credential helper, and therefore accepts "get" and "store" command-line arguments along with input data as described in the Git documentation: https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267 The Serialize() method of our new "credential" structure now encapsulates the logic that determines which credential fields to output in response to a command invocation with the "get" argument. Before commit 8e6641b, this logic was implemented in the fill() function, which handles "get" requests. When handling "get" requests for which the "authtype" credential field was not supplied (and no "authtype" capability was announced by the caller), and the credential record file read by the helper program does not indicate a response should be skipped, the current logic of the Serialize() method only outputs "username" and "password" fields if they were both found in the record file and are non-empty strings. However, the "clone ClientCert" test in our t/t-clone.sh script depends on credential record files we create in the directory identified by the CREDSDIR variable and which have empty values for the "username" field. These are used to supply the password for the encrypted certificate key file generated by our gitserver-lfstest utility program. In particular, when this test is run to completion, it executes a "git lfs clone" command using the URL on which our gitserver-lfstest program requires a client TLS/SSL certificate. We use the http.<url>.ssl{Cert,Key} configuration options to specify the client certificate files to Git. To decrypt the certificate, Git makes a request to the configured credential helper, sending empty "host" and "username" fields, a "path" field with the path to the certificate file, and a "protocol" field with the "cert" scheme. It expects our git-credential-lfstest helper to then return a "password" field with the passphrase for the encrypted client certificate file. Because the Serialize() method in our helper program now only returns "username" and "password" fields if they are both non-empty, it returns only the "host", "protocol", and "path" fields, without a "password" field. This prevents the test from succeeding, as Git then requests a password from the terminal since no helper provided one. Note, though, that due to the issue described in git-lfs#5658, this test does not actually run to completion at the moment, so we don't experience this problem in our CI jobs. We will address this issue in a subsequent commit in this PR, but as a first step, we need to at least restore the behaviour of our git-credential-lfstest helper program in the case where a credential record's file contains an empty "username" field and a non-empty "password" field. The original version of this code was introduced in 2015 in commit 5914d7d of PR git-lfs#306; it simply returned a "username" field if the input fields contained a "username" field, and the same for the "password" field. Prior to commit 8e6641b of PR git-lfs#5803 the helper program had evolved to handle some credential record files with formats other than simple "username" and "password" fields, but when returning those fields, continued to function like the original code in that it would return each field if there was a matching field among the input key/value pairs. Neither this approach, nor the current one, actually reflects how Git's own basic git-credential-store helper functions, nor others such as the git-credential-osxkeychain helper program. The git-credential-store program uses Git's credential_match() function to determine whether to return "username" and "password" fields for a given request, passing a zero (false) value for the match_password flag. For each of the "protocol", "host", "path", and "username" fields (but not the "password" field, because match_password is false) the credential_match() function checks if the field was provided in the request, and if so, that it matches the corresponding field in the credential record under consideration, assuming that field is also defined. Note that these fields may be defined but empty (i.e., be a pointer to zero-length string). If all the possible matches are met, then the function returns a non-zero (true) value. https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98 The git-credential-osxkeychain helper program is similar in that if a "username" field is provided, it must match the stored record before anything is returned, even if both are empty strings. But if a "username" field is not provided, both it and the "password" field will be returned, assuming the other required fields like "host" and "protocol" match. To make our git-credential-lfstest helper more accurately reflect the behaviour of these real credential helpers, and to resolve the problem with our "clone ClientCert" test and Git's inability to retrieve the passphrase for our encrypted test TLS/SSL client certificate, we therefore revise the Serialize() method of our "credential" Go structure so that before returning a "password" field, it checks whether a "username" field was provided, and if it was, that it matches the "username" field of the credential record (even if both are empty). As well, if a "username" field was not provided, then the one in the credential record is returned along with the "password" field. Further, we do not require that either the "username" or "password" fields in the credential record be non-empty. This brings our test credential helper program into closer alignment with the actions of Git's own helpers, and again allows a request for a credential with an empty "username" field in the input to match a credential record with an empty "username" field. One consequence of these changes on their own is that we might now return empty "username" and "password" fields even when the credential record file does not specify either, but had a leading non-empty field and so is a record we parse as one with fields for "authtype" and "credential" data. For example, if the credential record file contains the line "foo::bar", then we understand it has having an "authtype" field with the value "foo" and a "credential" field with the value of "bar". If a request is made which lacks "authtype" and "capability[]" fields, but whose "host" field (and possibly also "path" field) map to this credential record file, we will now return "host", "username", and "password" fields, with the latter having two empty values, and nothing else. This is because the Serialize() method will only return "authtype" and "credential" fields if the request contains an "authtype" field, and a "capability[]" field with the value "authtype", and if the credential record also contains non-empty "authtype" and "credential" fields. When these conditions are not met, the method falls through to the conditional governing whether "username" and "password" fields are output, and with our new conditions for handling those, we now allow for a missing "username" input field. Hence, without additional changes, we would output empty "username" and "password" fields in such a case as described above. To be clear, this is not something which occurs with our test suite as it stands now. Neverthless, we should ensure that we only output "username" and "password" fields if we have parsed the credential record file as having them. Therefore we add a check to the final conditional in the Serialze() method's logic, to ensure that we require an empty "authtype" field (i.e, a record such as ":foo:bar", with a blank leading field) before we will output "username" and "password" fields. In other words, we must have parsed the credential record file as one containing "username" and "password" field data before we will send those fields back to a caller.

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.

In commit 8e6641b of PR git-lfs#5803 we refactored our git-credential-lfstest test utility program so as to use a new internal "credential" structure to store the details of a given credential, as part of that PR's changes to support multi-stage authentication in a manner similar to that introduced in Git version 2.46.0. The git-credential-lfstest program acts as a Git credential helper, and therefore accepts "get" and "store" command-line arguments along with input data as described in the Git documentation: https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267 The Serialize() method of our new "credential" structure now encapsulates the logic that determines which credential fields to output in response to a command invocation with the "get" argument. Before commit 8e6641b, this logic was implemented in the fill() function, which handles "get" requests. When handling "get" requests for which the "authtype" credential field was not supplied (and no "authtype" capability was announced by the caller), and the credential record file read by the helper program does not indicate a response should be skipped, the current logic of the Serialize() method only outputs "username" and "password" fields if they were both found in the record file and are non-empty strings. However, the "clone ClientCert" test in our t/t-clone.sh script depends on credential record files we create in the directory identified by the CREDSDIR variable and which have empty values for the "username" field. These are used to supply the password for the encrypted certificate key file generated by our gitserver-lfstest utility program. In particular, when this test is run to completion, it executes a "git lfs clone" command using the URL on which our gitserver-lfstest program requires a client TLS/SSL certificate. We use the http.<url>.ssl{Cert,Key} configuration options to specify the client certificate files to Git. To decrypt the certificate, Git makes a request to the configured credential helper, sending empty "host" and "username" fields, a "path" field with the path to the certificate file, and a "protocol" field with the "cert" scheme. It expects our git-credential-lfstest helper to then return a "password" field with the passphrase for the encrypted client certificate file. Because the Serialize() method in our helper program now only returns "username" and "password" fields if they are both non-empty, it returns only the "host", "protocol", and "path" fields, without a "password" field. This prevents the test from succeeding, as Git then requests a password from the terminal since no helper provided one. Note, though, that due to the issue described in git-lfs#5658, this test does not actually run to completion at the moment, so we don't experience this problem in our CI jobs. We will address this issue in a subsequent commit in this PR, but as a first step, we need to at least restore the behaviour of our git-credential-lfstest helper program in the case where a credential record's file contains an empty "username" field and a non-empty "password" field. The original version of this code was introduced in 2015 in commit 5914d7d of PR git-lfs#306; it simply returned a "username" field if the input fields contained a "username" field, and the same for the "password" field. Prior to commit 8e6641b of PR git-lfs#5803 the helper program had evolved to handle some credential record files with formats other than simple "username" and "password" fields, but when returning those fields, continued to function like the original code in that it would return each field if there was a matching field among the input key/value pairs. Neither this approach, nor the current one, actually reflects how Git's own basic git-credential-store helper functions, nor others such as the git-credential-osxkeychain helper program. The git-credential-store program uses Git's credential_match() function to determine whether to return "username" and "password" fields for a given request, passing a zero (false) value for the match_password flag. For each of the "protocol", "host", "path", and "username" fields (but not the "password" field, because match_password is false) the credential_match() function checks if the field was provided in the request, and if so, that it matches the corresponding field in the credential record under consideration, assuming that field is also defined. Note that these fields may be defined but empty (i.e., be a pointer to zero-length string). If all the possible matches are met, then the function returns a non-zero (true) value. https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98 The git-credential-osxkeychain helper program is similar in that if a "username" field is provided, it must match the stored record before anything is returned, even if both are empty strings. But if a "username" field is not provided, both it and the "password" field will be returned, assuming the other required fields like "host" and "protocol" match. To make our git-credential-lfstest helper more accurately reflect the behaviour of these real credential helpers, and to resolve the problem with our "clone ClientCert" test and Git's inability to retrieve the passphrase for our encrypted test TLS/SSL client certificate, we therefore revise the Serialize() method of our "credential" Go structure so that before returning a "password" field, it checks whether a "username" field was provided, and if it was, that it matches the "username" field of the credential record (even if both are empty). As well, if a "username" field was not provided, then the one in the credential record is returned along with the "password" field. Further, we do not require that either the "username" or "password" fields in the credential record be non-empty. This brings our test credential helper program into closer alignment with the actions of Git's own helpers, and again allows a request for a credential with an empty "username" field in the input to match a credential record with an empty "username" field. One consequence of these changes on their own is that we might now return empty "username" and "password" fields even when the credential record file does not specify either, but had a leading non-empty field and so is a record we parse as one with fields for "authtype" and "credential" data. For example, if the credential record file contains the line "foo::bar", then we understand it has having an "authtype" field with the value "foo" and a "credential" field with the value of "bar". If a request is made which lacks "authtype" and "capability[]" fields, but whose "host" field (and possibly also "path" field) map to this credential record file, we will now return "host", "username", and "password" fields, with the latter having two empty values, and nothing else. This is because the Serialize() method will only return "authtype" and "credential" fields if the request contains an "authtype" field, and a "capability[]" field with the value "authtype", and if the credential record also contains non-empty "authtype" and "credential" fields. When these conditions are not met, the method falls through to the conditional governing whether "username" and "password" fields are output, and with our new conditions for handling those, we now allow for a missing "username" input field. Hence, without additional changes, we would output empty "username" and "password" fields in such a case as described above. To be clear, this is not something which occurs with our test suite as it stands now. Neverthless, we should ensure that we only output "username" and "password" fields if we have parsed the credential record file as having them. Therefore we add a check to the final conditional in the Serialze() method's logic, to ensure that we require an empty "authtype" field (i.e, a record such as ":foo:bar", with a blank leading field) before we will output "username" and "password" fields. In other words, we must have parsed the credential record file as one containing "username" and "password" field data before we will send those fields back to a caller.

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.

In commit 8e6641b of PR git-lfs#5803 we refactored our git-credential-lfstest test utility program so as to use a new internal "credential" structure to store the details of a given credential, as part of that PR's changes to support multi-stage authentication in a manner similar to that introduced in Git version 2.46.0. The git-credential-lfstest program acts as a Git credential helper, and therefore accepts "get" and "store" command-line arguments along with input data as described in the Git documentation: https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267 The Serialize() method of our new "credential" structure now encapsulates the logic that determines which credential fields to output in response to a command invocation with the "get" argument. Before commit 8e6641b, this logic was implemented in the fill() function, which handles "get" requests. When handling "get" requests for which the "authtype" credential field was not supplied (and no "authtype" capability was announced by the caller), and the credential record file read by the helper program does not indicate a response should be skipped, the current logic of the Serialize() method only outputs "username" and "password" fields if they were both found in the record file and are non-empty strings. However, the "clone ClientCert" test in our t/t-clone.sh script depends on credential record files we create in the directory identified by the CREDSDIR variable and which have empty values for the "username" field. These are used to supply the password for the encrypted certificate key file generated by our gitserver-lfstest utility program. In particular, when this test is run to completion, it executes a "git lfs clone" command using the URL on which our gitserver-lfstest program requires a client TLS/SSL certificate. We use the http.<url>.ssl{Cert,Key} configuration options to specify the client certificate files to Git. To decrypt the certificate, Git makes a request to the configured credential helper, sending empty "host" and "username" fields, a "path" field with the path to the certificate file, and a "protocol" field with the "cert" scheme. It expects our git-credential-lfstest helper to then return a "password" field with the passphrase for the encrypted client certificate file. Because the Serialize() method in our helper program now only returns "username" and "password" fields if they are both non-empty, it returns only the "host", "protocol", and "path" fields, without a "password" field. This prevents the test from succeeding, as Git then requests a password from the terminal since no helper provided one. Note, though, that due to the issue described in git-lfs#5658, this test does not actually run to completion at the moment, so we don't experience this problem in our CI jobs. We will address this issue in a subsequent commit in this PR, but as a first step, we need to at least restore the behaviour of our git-credential-lfstest helper program in the case where a credential record's file contains an empty "username" field and a non-empty "password" field. The original version of this code was introduced in 2015 in commit 5914d7d of PR git-lfs#306; it simply returned a "username" field if the input fields contained a "username" field, and the same for the "password" field. Prior to commit 8e6641b of PR git-lfs#5803 the helper program had evolved to handle some credential record files with formats other than simple "username" and "password" fields, but when returning those fields, continued to function like the original code in that it would return each field if there was a matching field among the input key/value pairs. Neither this approach, nor the current one, actually reflects how Git's own basic git-credential-store helper functions, nor others such as the git-credential-osxkeychain helper program. The git-credential-store program uses Git's credential_match() function to determine whether to return "username" and "password" fields for a given request, passing a zero (false) value for the match_password flag. For each of the "protocol", "host", "path", and "username" fields (but not the "password" field, because match_password is false) the credential_match() function checks if the field was provided in the request, and if so, that it matches the corresponding field in the credential record under consideration, assuming that field is also defined. Note that these fields may be defined but empty (i.e., be a pointer to zero-length string). If all the possible matches are met, then the function returns a non-zero (true) value. https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98 The git-credential-osxkeychain helper program is similar in that if a "username" field is provided, it must match the stored record before anything is returned, even if both are empty strings. But if a "username" field is not provided, both it and the "password" field will be returned, assuming the other required fields like "host" and "protocol" match. To make our git-credential-lfstest helper more accurately reflect the behaviour of these real credential helpers, and to resolve the problem with our "clone ClientCert" test and Git's inability to retrieve the passphrase for our encrypted test TLS/SSL client certificate, we therefore revise the Serialize() method of our "credential" Go structure so that before returning a "password" field, it checks whether a "username" field was provided, and if it was, that it matches the "username" field of the credential record (even if both are empty). As well, if a "username" field was not provided, then the one in the credential record is returned along with the "password" field. Further, we do not require that either the "username" or "password" fields in the credential record be non-empty. This brings our test credential helper program into closer alignment with the actions of Git's own helpers, and again allows a request for a credential with an empty "username" field in the input to match a credential record with an empty "username" field. One consequence of these changes on their own is that we might now return empty "username" and "password" fields even when the credential record file does not specify either, but had a leading non-empty field and so is a record we parse as one with fields for "authtype" and "credential" data. For example, if the credential record file contains the line "foo::bar", then we understand it has having an "authtype" field with the value "foo" and a "credential" field with the value of "bar". If a request is made which lacks "authtype" and "capability[]" fields, but whose "host" field (and possibly also "path" field) map to this credential record file, we will now return "host", "username", and "password" fields, with the latter having two empty values, and nothing else. This is because the Serialize() method will only return "authtype" and "credential" fields if the request contains an "authtype" field, and a "capability[]" field with the value "authtype", and if the credential record also contains non-empty "authtype" and "credential" fields. When these conditions are not met, the method falls through to the conditional governing whether "username" and "password" fields are output, and with our new conditions for handling those, we now allow for a missing "username" input field. Hence, without additional changes, we would output empty "username" and "password" fields in such a case as described above. To be clear, this is not something which occurs with our test suite as it stands now. Neverthless, we should ensure that we only output "username" and "password" fields if we have parsed the credential record file as having them. Therefore we add a check to the final conditional in the Serialze() method's logic, to ensure that we require an empty "authtype" field (i.e, a record such as ":foo:bar", with a blank leading field) before we will output "username" and "password" fields. In other words, we must have parsed the credential record file as one containing "username" and "password" field data before we will send those fields back to a caller.

In commit 8e6641b of PR git-lfs#5803 we refactored our git-credential-lfstest test utility program so as to use a new internal "credential" structure to store the details of a given credential, as part of that PR's changes to support multi-stage authentication in a manner similar to that introduced in Git version 2.46.0. The git-credential-lfstest program acts as a Git credential helper, and therefore accepts "get" and "store" command-line arguments along with input data as described in the Git documentation: https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267 The Serialize() method of our new "credential" structure now encapsulates the logic that determines which credential fields to output in response to a command invocation with the "get" argument. Before commit 8e6641b, this logic was implemented in the fill() function, which handles "get" requests. When handling "get" requests for which the "authtype" credential field was not supplied (and no "authtype" capability was announced by the caller), and the credential record file read by the helper program does not indicate a response should be skipped, the current logic of the Serialize() method only outputs "username" and "password" fields if they were both found in the record file and are non-empty strings. However, the "clone ClientCert" test in our t/t-clone.sh script depends on credential record files we create in the directory identified by the CREDSDIR variable and which have empty values for the "username" field. These are used to supply the password for the encrypted certificate key file generated by our gitserver-lfstest utility program. In particular, when this test is run to completion, it executes a "git lfs clone" command using the URL on which our gitserver-lfstest program requires a client TLS/SSL certificate. We use the http.<url>.ssl{Cert,Key} configuration options to specify the client certificate files to Git. To decrypt the certificate, Git makes a request to the configured credential helper, sending empty "host" and "username" fields, a "path" field with the path to the certificate file, and a "protocol" field with the "cert" scheme. It expects our git-credential-lfstest helper to then return a "password" field with the passphrase for the encrypted client certificate file. Because the Serialize() method in our helper program now only returns "username" and "password" fields if they are both non-empty, it returns only the "host", "protocol", and "path" fields, without a "password" field. This prevents the test from succeeding, as Git then requests a password from the terminal since no helper provided one. Note, though, that due to the issue described in git-lfs#5658, this test does not actually run to completion at the moment, so we don't experience this problem in our CI jobs. We will address this issue in a subsequent commit in this PR, but as a first step, we need to at least restore the behaviour of our git-credential-lfstest helper program in the case where a credential record's file contains an empty "username" field and a non-empty "password" field. The original version of this code was introduced in 2015 in commit 5914d7d of PR git-lfs#306; it simply returned a "username" field if the input fields contained a "username" field, and the same for the "password" field. Prior to commit 8e6641b of PR git-lfs#5803 the helper program had evolved to handle some credential record files with formats other than simple "username" and "password" fields, but when returning those fields, continued to function like the original code in that it would return each field if there was a matching field among the input key/value pairs. Neither this approach, nor the current one, actually reflects how Git's own basic git-credential-store helper functions, nor others such as the git-credential-osxkeychain helper program. The git-credential-store program uses Git's credential_match() function to determine whether to return "username" and "password" fields for a given request, passing a zero (false) value for the match_password flag. For each of the "protocol", "host", "path", and "username" fields (but not the "password" field, because match_password is false) the credential_match() function checks if the field was provided in the request, and if so, that it matches the corresponding field in the credential record under consideration, assuming that field is also defined. Note that these fields may be defined but empty (i.e., be a pointer to zero-length string). If all the possible matches are met, then the function returns a non-zero (true) value. https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34 https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98 The git-credential-osxkeychain helper program is similar in that if a "username" field is provided, it must match the stored record before anything is returned, even if both are empty strings. But if a "username" field is not provided, both it and the "password" field will be returned, assuming the other required fields like "host" and "protocol" match. To make our git-credential-lfstest helper more accurately reflect the behaviour of these real credential helpers, and to resolve the problem with our "clone ClientCert" test and Git's inability to retrieve the passphrase for our encrypted test TLS/SSL client certificate, we therefore revise the Serialize() method of our "credential" Go structure so that before returning a "password" field, it checks whether a "username" field was provided, and if it was, that it matches the "username" field of the credential record (even if both are empty). As well, if a "username" field was not provided, then the one in the credential record is returned along with the "password" field. Further, we do not require that either the "username" or "password" fields in the credential record be non-empty. This brings our test credential helper program into closer alignment with the actions of Git's own helpers, and again allows a request for a credential with an empty "username" field in the input to match a credential record with an empty "username" field. One consequence of these changes on their own is that we might now return empty "username" and "password" fields even when the credential record file does not specify either, but had a leading non-empty field and so is a record we parse as one with fields for "authtype" and "credential" data. For example, if the credential record file contains the line "foo::bar", then we understand it has having an "authtype" field with the value "foo" and a "credential" field with the value of "bar". If a request is made which lacks "authtype" and "capability[]" fields, but whose "host" field (and possibly also "path" field) map to this credential record file, we will now return "host", "username", and "password" fields, with the latter two having empty values, and nothing else. This is because the Serialize() method will only return "authtype" and "credential" fields if the request contains an "authtype" field, and a "capability[]" field with the value "authtype", and if the credential record also contains non-empty "authtype" and "credential" fields. When these conditions are not met, the method falls through to the conditional governing whether "username" and "password" fields are output, and with our new conditions for handling those, we now allow for a missing "username" input field. Hence, without additional changes, we would output empty "username" and "password" fields in such a case as described above. To be clear, this is not something which occurs with our test suite as it stands now. Nevertheless, we should ensure that we only output "username" and "password" fields if we have parsed the credential record file as having them. Therefore we add a check to the final conditional in the Serialize() method's logic, to ensure that we require an empty "authtype" field (i.e, a record such as ":foo:bar", with a blank leading field) before we will output "username" and "password" fields. In other words, we must have parsed the credential record file as one containing "username" and "password" field data before we will send those fields back to a caller.

In commit 904145f of PR git-lfs#306 the original version of our t/t-happy-path.sh example test script was added, with an introductory comment describing the one test that was in the file at the time as a sample test of the Git LFS client. We have subsequently copied that comment into several other test scripts by accident, so we remove those now. We also move the comment down in the t/t-happy-path.sh script so it precedes the original "happy path" test, as the test script now contains a number of other tests which are not generic example tests.

In commit 904145f of PR git-lfs#306 the original version of our t/t-happy-path.sh example test script was added, with an introductory comment describing the one test that was in the file at the time as a sample test of the Git LFS client. We have since copied that comment into several other test scripts, likely by accident, so we remove those comments now. We also move the comment in the t/t-happy-path.sh script so it directly precedes the original "happy path" test, as the script now contains a number of other tests that are not generic tests intended to be used as examples.

technoweenie added 5 commits May 14, 2015 10:55

only build lfs and commands sub packages by default.

5e78bce

ignore the new tests subdir

initial readme

da2a961

initial crappy sh test

5fc4fde

get test working with a git and lfs server

e2afd37

rename tests => test

b685624

technoweenie mentioned this pull request May 18, 2015

Fixed detection of LocalGitDir within submodules #312

Merged

technoweenie added 15 commits May 18, 2015 13:44

import testlib with as few changes as possible

b1f5025

test/local is not used anymore

667cde0

run the happy path test in the ci suite

9523561

add a single command to run integration tests

8a97acc

use a while loop

856779f

implement the full push/clone workflow test

5914d7d

better test output

3f15399

teach script/integration how to setup and run the test suite

7b6d89d

unsaved

2310602

teach script/integration how to clean up

b4a7bf8

double brackets

d17948c

single =

eb54207

run tests in parallel with xargs

de1e0a0

fail fast if script/test fails

b2a6fba

dont show the logs and env if the test passes

1b36588

michael-k reviewed May 19, 2015
View reviewed changes

technoweenie added 2 commits May 19, 2015 15:13

dont exit 0, this affects the exit status of the test

d9e0b22

better go conventions

cc0ba8a

This was referenced May 19, 2015

Integration Tests 2.0 #291

Closed

Fsck command #290

Closed

lots of test docs

904145f

integration tests cleanup after themselves unless told not to

98407dd

technoweenie changed the title ~~WIP Integration tests 3 (one more time with feeling)~~ Integration tests 3 (one more time with feeling) May 21, 2015

dump the git lfs version if it fails

6d811de

technoweenie added 2 commits May 21, 2015 10:38

report the git lfs version at the beginning of the script

c01bb2c

bring the old credential helper back to see if ci is more reliable

3bdfa6c

technoweenie added a commit that referenced this pull request May 21, 2015

Merge pull request #306 from github/integration-tests-3

6ba0e21

Integration tests 3 (one more time with feeling)

technoweenie merged commit 6ba0e21 into master May 21, 2015

technoweenie deleted the integration-tests-3 branch May 21, 2015 17:15

This was referenced May 21, 2015

Michael k panic fix #319

Merged

ssh:// style URLs not correctly handled #295

Closed

technoweenie mentioned this pull request Jun 11, 2015

Release v0.5.2 #281

Closed

38 tasks

epriestley mentioned this pull request Mar 18, 2016

Accept raw remote URLs as valid #1085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Integration tests 3 (one more time with feeling) #306

Integration tests 3 (one more time with feeling) #306

Uh oh!

technoweenie commented May 14, 2015

Uh oh!

michael-k May 19, 2015

Uh oh!

technoweenie May 19, 2015

Uh oh!

technoweenie commented May 19, 2015

Uh oh!

michael-k commented May 21, 2015

Uh oh!

technoweenie commented May 21, 2015

Uh oh!

technoweenie commented May 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Integration tests 3 (one more time with feeling) #306

Integration tests 3 (one more time with feeling) #306

Uh oh!

Conversation

technoweenie commented May 14, 2015

Uh oh!

michael-k May 19, 2015

Choose a reason for hiding this comment

Uh oh!

technoweenie May 19, 2015

Choose a reason for hiding this comment

Uh oh!

technoweenie commented May 19, 2015

Uh oh!

michael-k commented May 21, 2015

Uh oh!

technoweenie commented May 21, 2015

Uh oh!

technoweenie commented May 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants