Thread: Re: Add TAP test for auth_delay extension
Hi Hackers, I just wrote a test for `auth_delay` extension. It's a test which confirms whether there is a delay for a second when you enter the wrong password. I sent an email using mutt, but I have a problem and sent it again. --- Regards, Dong Wook Lee.
2022년 6월 7일 (화) 오후 6:32, Dong Wook Lee <sh95119@gmail.com>님이 작성: > > Hi Hackers, > I just wrote a test for `auth_delay` extension. > It's a test which confirms whether there is a delay for a second when > you enter the wrong password. > I sent an email using mutt, but I have a problem and sent it again. > > --- > Regards, > Dong Wook Lee. Hi, I have written a test for the auth_delay extension before, but if it is okay, can you review it? --- Regards, DongWook Lee.
On Sat, Jun 18, 2022 at 11:06:02AM +0900, Dong Wook Lee wrote: > I have written a test for the auth_delay extension before, > but if it is okay, can you review it? +# check enter wrong password +my $t0 = [gettimeofday]; +test_login($node, 'user_role', "wrongpass", 2); +my $elapsed = tv_interval($t0, [gettimeofday]); +ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); + +# check enter correct password +my $t0 = [gettimeofday]; +test_login($node, 'user_role', "pass", 0); +my $elapsed = tv_interval($t0, [gettimeofday]); +ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); On a slow machine, I suspect that the second test is going to be unstable as it would fail if the login attempt (that succeeds) takes more than $delay_milliseconds. You could increase more delay_milliseconds to leverage that, but it would make the first test slower for nothing on faster machines in the case where the authentication attempt has failed. I guess that you could leverage that by using a large value for delay_milliseconds in the second test, because we are never going to wait. For the first test, you could on the contrary use a much lower value, still on slow machines it may not test what the code path of auth_delay you are willing to test. As a whole, I am not sure that this is really worth spending cycles on when running check-world or similar, and the code of the extension is trivial. -- Michael
Attachment
On 22/06/18 12:07오후, Michael Paquier wrote: > On Sat, Jun 18, 2022 at 11:06:02AM +0900, Dong Wook Lee wrote: > > I have written a test for the auth_delay extension before, > > but if it is okay, can you review it? > > +# check enter wrong password > +my $t0 = [gettimeofday]; > +test_login($node, 'user_role', "wrongpass", 2); > +my $elapsed = tv_interval($t0, [gettimeofday]); > +ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); > + > +# check enter correct password > +my $t0 = [gettimeofday]; > +test_login($node, 'user_role', "pass", 0); > +my $elapsed = tv_interval($t0, [gettimeofday]); > +ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); > > On a slow machine, I suspect that the second test is going to be > unstable as it would fail if the login attempt (that succeeds) takes > more than $delay_milliseconds. You could increase more > delay_milliseconds to leverage that, but it would make the first test > slower for nothing on faster machines in the case where the > authentication attempt has failed. I guess that you could leverage > that by using a large value for delay_milliseconds in the second test, > because we are never going to wait. For the first test, you could on > the contrary use a much lower value, still on slow machines it may not > test what the code path of auth_delay you are willing to test. > Thank you for your valuable advice I didn't think about the slow system. Therefore, in the case of the second test, the time was extended a little. > As a whole, I am not sure that this is really worth spending cycles on > when running check-world or similar, and the code of the extension is > trivial. Even though it is trivial, I think it would be better if there was a test. > -- > Michael
Attachment
Dong Wook Lee <sh95119@gmail.com> writes:
> On 22/06/18 12:07오후, Michael Paquier wrote:
>> As a whole, I am not sure that this is really worth spending cycles on
>> when running check-world or similar, and the code of the extension is
>> trivial.
> Even though it is trivial, I think it would be better if there was a test.
I looked at this and concur with Michael's evaluation. A new TAP module
is quite an expensive thing, since it incurs (at least) an initdb run.
In this case, the need to delay a long time to ensure that the test
doesn't fail on slow systems makes that even worse. I don't think
I want to incur these costs every time I run check-world in order to
test a pg_usleep() call, which is what this module boils down to.
If we had some sort of "attic" of tests that aren't run by either
check-world or most buildfarm members, perhaps this would be worth
putting there. But we don't.
One idea could be to install the test but leave the TAP_TESTS line in
the Makefile commented out. Then, somebody who was actively working on
the module could enable the test easily enough (without even modifying
that file: just do "make check TAP_TESTS=1"), but otherwise we don't
pay for it. However, I'm not sure how well that plan will translate
to the upcoming meson build system.
If we don't do it like that, I'd vote for rejecting the patch.
regards, tom lane
On Sat, Jul 30, 2022 at 05:13:12PM -0400, Tom Lane wrote: > If we don't do it like that, I'd vote for rejecting the patch. Yep. Done this way. -- Michael