Re: Better testing coverage and unified coding for plpgsql loops - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: Better testing coverage and unified coding for plpgsql loops
Date
Msg-id CAFj8pRBRwMhDQ2bwypQMEmpS7CrWOYkFWDN_Qt5AGEvPDbH13g@mail.gmail.com
Whole thread Raw
In response to Better testing coverage and unified coding for plpgsql loops  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers


2017-12-30 22:46 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
While I've been fooling around with plpgsql, I've been paying close
attention to code coverage reports to make sure that the regression tests
exercise all that new code.  It started to bug me that there were some
serious gaps in the test coverage for existing code in pl_exec.c.
One thing I noticed in particular was that coverage for the
PLPGSQL_RC_EXIT/PLPGSQL_RC_RETURN/PLPGSQL_RC_CONTINUE management code
in the various looping statements was nearly nonexistent, and coverage
for integer FOR loops was pretty awful too (eg, not a single BY clause
in the whole test corpus :-().  So I said to myself "let's fix that",
and wrote up a new regression test file plpgsql_control.sql with a
charter to test plpgsql's control structures.  I moved a few existing
tests that seemed to fall into that charter into the new file, and
added tests until I was happier with the state of the coverage report.
The result is attached as plpgsql-better-code-coverage.patch.

However, while I was doing that, it seemed like the tests I was adding
were mighty repetitive, as many of them were just exactly the same thing
adjusted for a different kind of loop statement.  And so I began to wonder
why it was that we had five copies of the RC_FOO management logic, no two
quite alike.  If we only had *one* copy then it would not seem necessary
to have such duplicative test cases for it.  A bit of hacking later, and
I had the management logic expressed as a macro, with only one copy for
all five kinds of loop.  I verified it still passes the previous set of
tests and then removed the ones that seemed redundant, yielding
plpgsql-unify-loop-rc-code.patch below.  So what I propose actually
committing is the combination of these two patches.

Anyone feel like reviewing this, or should I just push it?  It seems
pretty noncontroversial to me, but maybe I'm wrong about that.

I don't think any issue there. This macro is little bit massive, but similar is used elsewhere.

+1

Pavel



                        regards, tom lane


pgsql-hackers by date:

Previous
From: Pavel Stehule
Date:
Subject: Re: Converting plpgsql to use DTYPE_REC for named composite types
Next
From: Fabien COELHO
Date:
Subject: Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME