On Wed, Jul 14 2021, Jonathan Tan wrote:
>> > +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.
>
>>
>> > + 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.
>
>>
>> > + 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:
>>
>> > + 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).
>>
>> 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.
>
>>
>> > + 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.
>
>>
>> > +'
>> > +
>> > +# 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.
>>
>> * 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.
