Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>
> Add a series of tests to exercise the HTTP authentication header parsing
> and the interop with credential helpers. Credential helpers will receive
> WWW-Authenticate information in credential requests.
A general comment about this series - the way you have the patches organized
means that the "feature" content you're trying to integrate (the first two
patches) is contextually separated from these tests. For people that
learn/understand code via examples in tests, this makes it really difficult
to understand what's going on. To avoid that, I think you could rearrange
the patches pretty easily:
1. test-http-server: add stub HTTP server test helper (prev. patch 3)
- t5556 could be introduced here with the basic "anonymous" test in patch
6, but marked 'test_expect_failure'.
2. test-http-server: add HTTP error response function (prev. patch 4)
3. test-http-server: add HTTP request parsing (prev. patch 5)
4. test-http-server: pass Git requests to http-backend (prev. patch 6)
5. test-http-server: add simple authentication (prev. patch 7)
6. http: read HTTP WWW-Authenticate response headers (prev. patch 1)
7. credential: add WWW-Authenticate header to cred requests (prev patch 2)
- Some/all of the tests from the current patch (patch 8) could be squashed
into this one so that the tests exist directly alongside the new
functionality they're testing.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
> ---
> t/helper/test-credential-helper-replay.sh | 14 +++
> t/t5556-http-auth.sh | 120 +++++++++++++++++++++-
> 2 files changed, 133 insertions(+), 1 deletion(-)
> create mode 100755 t/helper/test-credential-helper-replay.sh
>
> diff --git a/t/helper/test-credential-helper-replay.sh b/t/helper/test-credential-helper-replay.sh
> new file mode 100755
> index 00000000000..03e5e63dad6
> --- /dev/null
> +++ b/t/helper/test-credential-helper-replay.sh
I'm not sure a 't/helper' file is the right place for this - it's a pretty
simple shell script, but it defines a lot of information (namely 'teefile',
'catfile') that is otherwise unexplained in 't5556'.
What about something like 'lib-rebase.sh' and its 'set_fake_editor()'? You
could create a similar test lib ('lib-credential-helper.sh') and wrapper
function (' that writes out a custom credential helper. Something like
'set_fake_credential_helper()' could also take 'teefile' and 'catfile' as
arguments, making their names more transparent to 't5556'.
> @@ -0,0 +1,14 @@
> +cmd=$1
> +teefile=$cmd-actual.cred
> +catfile=$cmd-response.cred
> +rm -f $teefile
> +while read line;
> +do
> + if test -z "$line"; then
> + break;
> + fi
> + echo "$line" >> $teefile
> +done
> +if test "$cmd" = "get"; then
> + cat $catfile
> +fi
> diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
> index 78da151f122..541fa32bd77 100755
> --- a/t/t5556-http-auth.sh
> +++ b/t/t5556-http-auth.sh
> @@ -26,6 +26,8 @@ PID_FILE="$(pwd)"/pid-file.pid
> SERVER_LOG="$(pwd)"/OUT.server.log
>
> PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
> +CREDENTIAL_HELPER="$GIT_BUILD_DIR/t/helper/test-credential-helper-replay.sh" \
> + && export CREDENTIAL_HELPER
I see - this is how you connect the "test" credential helper to the HTTP
server and header parsing (as implemented in patches 1 & 2), so that the
results can be compared for correctness.
nit: you can just 'export CREDENTIAL_HELPER="..."', rather than breaking it
into two lines. You also shouldn't need to 'export' at all - the value will
be set in the context of the test.
>
> test_expect_success 'setup repos' '
> test_create_repo "$REPO_DIR" &&
> @@ -91,7 +93,8 @@ start_http_server () {
>
> per_test_cleanup () {
> stop_http_server &&
> - rm -f OUT.*
> + rm -f OUT.* &&
> + rm -f *.cred
> }
>
> test_expect_success 'http auth anonymous no challenge' '
> @@ -102,4 +105,119 @@ test_expect_success 'http auth anonymous no challenge' '
> git ls-remote $ORIGIN_URL
> '
>
> +test_expect_success 'http auth www-auth headers to credential helper basic valid' '
...
> +test_expect_success 'http auth www-auth headers to credential helper custom schemes' '
...
> +test_expect_success 'http auth www-auth headers to credential helper invalid' '
These tests all look good. That said, is there any way to test more
bizarre/edge cases (headers too long to fit on one line, headers that end
with a long string of whitespace, etc.)?
