diff mbox series

debuginfod: Fracture tests/run-debuginfod-find.sh into specific tests

Message ID 8608ab7d03f5ceda0f45a4175554e2185c48ab4d.camel@klomp.org
State Superseded
Headers show
Series debuginfod: Fracture tests/run-debuginfod-find.sh into specific tests | expand

Commit Message

Mark Wielaard Sept. 2, 2021, 1:42 p.m. UTC
Hi Noah,

I really like this split up of test cases.
I added the new x-forward-ttl testcase to it.
run-debuginfod-x-forwarded-for.sh

Two tests run-debuginfod-federation-metrics.sh and run-debuginfod-
federation-sqlite.sh called get_ports twice to get an "invalid" port
number to set DEBUGINFOD_URLS. I changed this to:
export DEBUGINFOD_URLS='http://127.0.0.1:0' # Note invalid

The only concern I have is with get_ports. There is a race condition
selecting the random port when lots of debuginfod tests are running. It
could hand out ports that are not yet used to multiple tests.

I only managed to trigger this once, with make check -j256. So it
probably isn't a big race, but getting random failures is bad.

I don't have a good solution for this though. Maybe we could assign an
unique range to each testcase? e.g.

And then add a unique base to each get_port call (8000, 8100, 8200,
...). We would to be careful to never assign the same unique range to
each test though. Which requires the programmer to check all other
tests. But maybe that isn't such a big deal? git grep ^get_port tests
would give you all the currently used port ranges...

Ideas?

Attached the slightly updated patch, including the new run-debuginfod-
x-forwarded-for.sh, also on 
https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=debuginfod-fracture-tests

Cheers,

Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debuginfod-Fracture-tests-run-debuginfod-find.sh-int.patch
Type: text/x-patch
Size: 130805 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210902/18a8521d/attachment-0001.bin>

Comments

Noah Sanci Sept. 2, 2021, 6:59 p.m. UTC | #1
Hello,

As for the rpm/Makefile.am stuff, I think that was a temporary fix for
an odd error
I got while testing. I just forgot to remove it and see if it would
break again. It has
been removed and makes now.

As for the 'base' idea, I gave it some thought and believe this is the
best solution
and I have implemented it.


> And then add a unique base to each get_port call (8000, 8100, 8200,
> ...). We would to be careful to never assign the same unique range to
> each test though. Which requires the programmer to check all other
> tests. But maybe that isn't such a big deal? git grep ^get_port tests
> would give you all the currently used port ranges...
>
> Ideas?
>
Agreed, implemented.

The patch also includes x-forwarded

-Noah Sanci
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debuginfod-Fracture-tests-run-debuginfod-find.sh-int.patch
Type: text/x-patch
Size: 106190 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210902/23fd2d97/attachment-0001.bin>
Mark Wielaard Sept. 3, 2021, 8:45 a.m. UTC | #2
Hi Noah,

On Thu, Sep 02, 2021 at 02:59:06PM -0400, Noah Sanci wrote:
> As for the rpm/Makefile.am stuff, I think that was a temporary fix for
> an odd error  I got while testing. I just forgot to remove it and see if
> it would break again. It has been removed and makes now.
>
> As for the 'base' idea, I gave it some thought and believe this is the
> best solution and I have implemented it.

Great, thanks.

BTW. You don't have to use globals in shell, shell function can take arguments.
They are just not named, so you then have to use $1, $2, etc to use them.
But this works too.

> The patch also includes x-forwarded

It even contained it twice :)
I removed the not really used copy run-debuginfod-X-FORWARDED.sh.

I also removed run-debuginfod-response-headers.sh for now since it
depends on a patch that isn't in yet.

Finally when testing on a system where /bin/sh isn't bash I noticed
that trap err ERR isn't posix shell. Sigh.

I moved the helpers into their own tests/debuginfod-subr.sh which is
then only called from the run-debuginfod-*.sh scripts (which are all
explicitly bash scripts). This does have the advantage that the extra
checks are only run when testing debuginfod and not any of the other
tests.

Attached the variant that I pushed. I hope I didn't mess anything up.

Thanks so much for splitting these tests, it is really great to have
individual test cases.

Cheers,

Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debuginfod-Fracture-tests-run-debuginfod-find.sh-int.patch
Type: text/x-diff
Size: 134951 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210903/227f7307/attachment-0001.bin>
diff mbox series

Patch

diff --git a/tests/test-subr.sh b/tests/test-subr.sh
index 41e95e31..880d6590 100644
--- a/tests/test-subr.sh
+++ b/tests/test-subr.sh
@@ -314,14 +314,17 @@  archive_test() {
     fi
 }
 
+# takes one argument a "base" port number, this should be unique between
+# tests to select (two) random port number between $base..$base+100.
 get_ports() {
+  base=$1
   while true; do
-    PORT1=`expr '(' $RANDOM % 1000 ')' + 9000`
+    PORT1=`expr '(' $RANDOM % 100 ')' + $base`
     ss -atn | fgrep ":$PORT1" || break
   done
 # Some tests will use two servers, so assign the second var
   while true; do
-    PORT2=`expr '(' $RANDOM % 1000 ')' + 9000`
+    PORT2=`expr '(' $RANDOM % 100 ')' + $base`
     ss -atn | fgrep ":$PORT2" && $PORT1 -ne $PORT2 || break
   done