On Wed, Jul 14 2021, Jonathan Tan wrote:

quoted

quoted

+test_expect_success 'http push with negotiation' '
+	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+	URI="$HTTPD_URL/smart/server" &&
+
+	rm -rf client &&

Nothing in this test file has created a "client" directory at this
point, so we shouldn't have this to remove it.

I think the real reason for this is that you're just copy/pasting this
from elsewhere. I've got an unsubmitted series where I fixed various
callsites in these t/t*http* tests to use test_when_finished instead.

This pattern of having the next test clean up after you leads to bad
inter-dependencies, there were a few things broken e.g. if earlier tests
were skipped. Let's not continue that pattern.

OK - I have changed it so that each test is independent and cleans up after
itself.

quoted

quoted

+	git init client &&
+	test_commit -C client first_commit &&
+	test_commit -C client second_commit &&
+
+	# Without negotiation
+	test_create_repo "$SERVER" &&

s/test_create_repo/git init/g

Done.

quoted

quoted

+	test_config -C "$SERVER" http.receivepack true &&
+	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
+	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
+	GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 \
+		push "$URI" refs/heads/main:refs/remotes/origin/main &&
+	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
+
+	# Same commands, but with negotiation
+	rm event &&
+	rm -rf "$SERVER" &&

Ditto test_when_finished, or actually:

quoted

+	test_create_repo "$SERVER" &&
+	test_config -C "$SERVER" http.receivepack true &&

In my current version, I have changed everything to "git init". Should I
use "test_create_repo" instead?

Sorry I just copy/pasted that from your version, yes indeed
s/test_create_repo/git init/ (a trivial thing, but they're 1=1
equivalent these days after a recent change of mine in
test-lib-functions.sh).

quoted

If we're entirely setting up the server again shouldn't this just be
another test_expect_success block?

I only made one block at first because the test without negotiation is
there just for comparing object counts, but OK, I have split it up into
2 blocks.

quoted

quoted

+	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
+	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
+	GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 -c push.negotiate=1 \
+		push "$URI" refs/heads/main:refs/remotes/origin/main &&
+	grep_wrote 3 event # 1 commit, 1 tree, 1 blob
+'
+
+test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '
+	rm event &&
+	rm -rf "$SERVER" &&
+	test_create_repo "$SERVER" &&
+	test_config -C "$SERVER" http.receivepack true &&
+	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
+	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
+	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c push.negotiate=1 \
+		push "$URI" refs/heads/main:refs/remotes/origin/main 2>err &&
+	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
+	test_i18ngrep "push negotiation failed" err

s/test_i18ngrep/grep/, or actually better yet is there a reason not to
do test_cmp here? I'm pretty sure there's not.

The reason I've got that unsubmitted series is because I found a bug
that was cleverly hidden away by an earlier such 'grep the output'
code/test you'd added recently.

There *are* cases where we e.g. get STDERR output from the OS itself
about bad connections, or races, but we should at least:

    grep error: <stderr >error-lines.actual &&
    test_cmp error-lines.expect error-lines.actual

To check that we get the errors we expected, and *only* those.

I didn't want to make the test prescribe unnecessary details, but good
point about hiding bugs. I have switched it to what you describe.

quoted

quoted

+'
+
+# DO NOT add non-httpd-specific tests here, because the last part of this
+# test script is only executed when httpd is available and enabled.
+
 test_done

Also, instead of this comment let's just create another
t*-fetch-push-http.sh test. Because:

 * This test is already really slow, it takes me 13s to run it now. I
   haven't tested, but setting up apache will make it even slower.

 * It's also an anti-pattern to switch midway to needing an external
   daemon v.s. doing it in a dedicated test.

   E.g. if you have the relevant GIT_* settings to run http:// tests,
   but not git:// tests we'll skip the former entirely in
   t5700-protocol-v1.sh, because we'll "skip all" as soon as we see we
   can't start the git-daemon.

   That specifically isn't an issue here, but better to avoid setting up
   the pattern.

I think this is a pattern that we need, though. Sometimes there are
closely related fetch/push tests (as in here, and as in t5700) that want
to test similar things across different protocols.

Yeah, in my split-up WIP there's a bit of that, in my split-up of them I
didn't find the end result harder to read, e.g. in
t/t5702-protocol-v2.sh it's mostly in different sections of the file,
first git://, then file:// and then http://, with some misc in-between,
having a t/t57*-protocol-{git,file,http}.sh didn't make things harder to
read.

quoted

 * What *is* an issue with it here is that the "skip all" in TAP is only
   handled before you run any tests, e.g. compare:
       
       $ prove ./t5700-protocol-v1.sh
       ./t5700-protocol-v1.sh .. ok    
       All tests successful.
       Files=1, Tests=21,  2 wallclock secs ( 0.02 usr  0.00 sys +  0.80 cusr  0.80 csys =  1.62 CPU)
       Result: PASS
       
       $ GIT_TEST_GIT_DAEMON=false GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh
       ./t5700-protocol-v1.sh .. skipped: git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)
       Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.02 cusr  0.00 csys =  0.03 CPU)
       Result: NOTESTS
       
       $ GIT_TEST_GIT_DAEMON=true GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh
       ./t5700-protocol-v1.sh .. ok    
       All tests successful.
       Files=1, Tests=16,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.63 cusr  0.59 csys =  1.23 CPU)
       Result: PASS
   
   I.e. if you stick an inclusion of lib-httpd.sh this late in a test
   file we won't get a prominent notice saying we're categorically
   skipping certain types of tests based on an external dependency.

   If you had your own dedicated test instead you'd get:
       
       $ GIT_TEST_HTTPD=false  prove ./t5705-protocol-v2-http.sh 
       ./t5705-protocol-v2-http.sh .. skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)
       Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.02 cusr  0.01 csys =  0.05 CPU)
       Result: NOTESTS

Is it useful to know the count of tests that are skipped? (The user
presumably already knows that some are skipped because they set the
environment variable in the first place.)

A thing I do really often is to run some glob of tests, say I'm working
on git-fetch, and run *fetch*, I'd get this in the output:
    
    [23:58:15] t5527-fetch-odd-refs.sh ......................... ok      121 ms ( 0.01 usr  0.00 sys +  0.72 cusr  0.38 csys =  1.11 CPU)
    [23:58:15] t5535-fetch-push-symref.sh ...................... ok       90 ms ( 0.01 usr  0.00 sys +  0.14 cusr  0.02 csys =  0.17 CPU)
    [23:58:16] t5536-fetch-conflicts.sh ........................ ok      180 ms ( 0.01 usr  0.01 sys +  0.28 cusr  0.07 csys =  0.37 CPU)
    [23:58:16] t5539-fetch-http-shallow.sh ..................... skipped: no web server found at '/usr/sbin/apache'
    [23:58:16] t5550-http-fetch-dumb.sh ........................ skipped: no web server found at '/usr/sbin/apache'
    [23:58:16] t5551-http-fetch-smart.sh ....................... skipped: no web server found at '/usr/sbin/apache'
    [23:58:16] t5554-noop-fetch-negotiator.sh .................. ok        3 ms ( 0.00 usr  0.00 sys +  0.07 cusr  0.02 csys =  0.09 CPU)
    [23:58:16] t5537-fetch-shallow.sh .......................... ok      641 ms ( 0.03 usr  0.01 sys +  0.87 cusr  0.31 csys =  1.22 CPU)
    [23:58:16] t5582-fetch-negative-refspec.sh ................. ok      564 ms ( 0.02 usr  0.01 sys +  0.77 cusr  0.32 csys =  1.12 CPU)

It's useful to know that a portion of the tests was skipped entirely,
whereas if we do git:// tests first, and then later http:// the loss of
coverage is silent (unless you run it individually under --verbose) or
whatever.

This is really useful for the http tests iin particular, it's really
common not to have apache installed.