On Tue, Nov 01, 2022 at 04:54:38PM -0400, Taylor Blau wrote: > > The tests aren't just asserting the bad behavior, they're also ensuring > > that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but > > tolerable, but if we start emitting 500 of these it would be nice to > > know. > > I admit that this kind of argument does not sway me. > > Is it likely that we would suddenly start spewing 500 such warnings? If > we did, are there no other tests that would catch it? And even if *that* > were the case, would nobody happen to notice it in the meantime either > during development or when we queue an affected topic onto 'next' for > wider testing? > > I guess the answer is that it's possible that we'd miss such a > regression in all of those above places, but to me it seems extremely > unlikely that we'd let such a regression through without noticing. Like you, I don't find much value in asserting "2 or 3, but not 500". But it is easy enough to at least only loosen the few cases that need it. So here's a replacement for 2/2 that does the minimal thing. I rewrote the commit message to explain my view (incidentally, it also fixes the subject line, which mentioned the wrong test number after the rebase). As I said, I had tried to mostly leave patch 2 alone to avoid derailing Dscho's attempt to fix things. But somehow things got derailed anyway, so maybe we can just all agree on this patch and move on with our lives? I can't over-emphasize how little I care about this credentialsInUrl feature in the first place, and somehow it has consumed hours of my life now. -- >8 -- Subject: t5551: be less strict about the number of credential warnings It is unclear as to _why_, but under certain circumstances the warning about credentials being passed as part of the URL seems to be swallowed by the `git remote-https` helper in the Windows jobs of Git's CI builds. This causes the tests to fail, because they assert that in a few cases we should print the warning 2 or even 3 times. The reason for that is given in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config, 2022-06-06), which is that multiple processes are involved, and each warns. In an ideal world, the user would just see the message once per logical operation; they don't care how many underlying processes are involved. And we may fix that eventually. But in the meantime, let's loosen the tests to just assert that the user sees the message _at least_ once. This won't catch a case where we accidentally start producing it 500 times, but that's not a likely regression. A more likely thing is that we'd start producing it four times because the underlying code changes to use a new process. But that's exactly the kind of thing we'd prefer to be ignoring in the tests. Note that the tests for the "die" mode don't need adjusted. They die immediately in the first process that sees the problem, so they consistently show the error once. It's only the "warn" mode which must be loose. If we eventually fix it, then we can tighten its assertion. In the meantime, this works around the CI issues. Based on a patch by Johannes Schindelin. Signed-off-by: Jeff King <peff@xxxxxxxx> --- t/t5551-http-fetch-smart.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index bbe3d5721f..4f559722f4 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -597,7 +597,7 @@ test_expect_success 'clone warns or fails when using username:password' ' git -c transfer.credentialsInUrl=warn \ clone $url_userpass attempt2 2>err && grep "warning: $message" err >warnings && - test_line_count = 2 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ clone $url_userpass attempt3 2>err && @@ -630,7 +630,7 @@ test_expect_success 'fetch warns or fails when using username:password' ' git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err && grep "warning: $message" err >warnings && - test_line_count = 3 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ fetch $url_userpass 2>err && -- 2.38.1.677.g9b1428ac2e
