diff mbox series

[v3,2/2] t5551: be less strict about the number of credential warnings

Message ID Y2ItZWx+kBmTreGQ@coredump.intra.peff.net (mailing list archive)
State New, archived
Headers show
Series [RFC] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings | expand

Commit Message

Jeff King Nov. 2, 2022, 8:42 a.m. UTC 
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@peff.net>
---
 t/t5551-http-fetch-smart.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Sunshine Nov. 2, 2022, 8:49 a.m. UTC | #1 
On Wed, Nov 2, 2022 at 4:46 AM Jeff King <peff@peff.net> wrote:
> 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

s/adjusted/adjustment --or -- s/need/& to be/

> 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@peff.net>
Jeff King Nov. 2, 2022, 9:15 a.m. UTC | #2 
On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote:

> > Note that the tests for the "die" mode don't need adjusted. They die
> 
> s/adjusted/adjustment --or -- s/need/& to be/

https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w

Don't stomp on my linguistic heritage. :)

-Peff
Jeff King Nov. 2, 2022, 9:18 a.m. UTC | #3 
On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote:

> 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?

By the way, I think you (or somebody?) mentioned elsewhere in the thread
that it's possible the first patch fixes things by itself. I would also
be content to just apply the first one and see if CI improves.

Of course, when I just pushed all this out to CI, it flaked
independently on both osx (looks like racy p4 stuff) and fedora (network
dropout failed to set up the container). Sigh.

-Peff
Eric Sunshine Nov. 2, 2022, 9:31 a.m. UTC | #4 
On Wed, Nov 2, 2022 at 5:15 AM Jeff King <peff@peff.net> wrote:
> On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote:
> > > Note that the tests for the "die" mode don't need adjusted. They die
> >
> > s/adjusted/adjustment --or -- s/need/& to be/
>
> https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w
>
> Don't stomp on my linguistic heritage. :)

Sorry. My head needs hanged in shame. I forgot that I can't grammar.
(I can't math either.)
Taylor Blau Nov. 3, 2022, 1:31 a.m. UTC | #5 
On Wed, Nov 02, 2022 at 05:18:29AM -0400, Jeff King wrote:
> On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote:
>
> > 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?
>
> By the way, I think you (or somebody?) mentioned elsewhere in the thread
> that it's possible the first patch fixes things by itself. I would also
> be content to just apply the first one and see if CI improves.
>
> Of course, when I just pushed all this out to CI, it flaked
> independently on both osx (looks like racy p4 stuff) and fedora (network
> dropout failed to set up the container). Sigh.

Dreams are free, eh?

Thanks,
Taylor
diff mbox series

Patch

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 &&