kputnam / piggly

PL/pgSQL stored procedure code coverage tool
Other
69 stars 14 forks source link

failing to capture calls to secondary procedure for perform call #38

Closed born-in-brooklyn closed 6 years ago

born-in-brooklyn commented 6 years ago

let's assume for a moment that the following procedure exists:

CREATE OR REPLACE FUNCTION cat.func_potato_fish(p_dog_cat_fish_id integer)
  RETURNS text AS
$BODY$
DECLARE
  v_dog_id INTEGER;
  v_state   TEXT;
  v_message TEXT;
BEGIN
  IF p_dog_cat_fish_id IS NULL THEN
    RAISE EXCEPTION 'Can not recalculate potato cat for fish if fish id passed is NULL';
  END IF;

  SELECT fr.dog_id
  INTO v_dog_id
  FROM cat.dog_cat fr
  INNER JOIN cat.dog_cat_fish fra
    ON fr.dog_cat_id = fra.dog_cat_id
  WHERE fra.dog_cat_fish_id = p_dog_cat_fish_id;

  PERFORM cat.func_potato_exct_nrmld_score(p_dog_cat_fish_id);
  PERFORM cat.func_potato_exct_dog_likelihood(v_dog_id);
  PERFORM cat.func_cmpst_exct_dog_score(v_dog_id);
  RETURN '';

  EXCEPTION
  WHEN OTHERS
  THEN
    GET STACKED DIAGNOSTICS v_message = MESSAGE_TEXT,
    v_state = RETURNED_SQLSTATE;
    RAISE WARNING 'Exception - % -ERROR- %', v_state, v_message;
    RETURN v_message;
END;
$BODY$
  LANGUAGE plpgsql VOLATILE
  COST 100;

let's further assume that a suite of pg_tap tests have been built to test this procedure, These tests overwrite the definitions of the dependent procedures, cat.func_potato_exct_nrmld_score, cat.func_potato_exct_dog_likelihood, cat.func_cmpst_exct_dog_score:

CREATE TEMPORARY TABLE calls_to__func_potato_exct_nrmld_score
(
  p_dog_cat_fish_id INTEGER
);

CREATE OR REPLACE FUNCTION cat.func_potato_exct_nrmld_score(p_dog_cat_fish_id INTEGER DEFAULT NULL)
  RETURNS INTEGER
LANGUAGE plpgsql
AS $$
BEGIN
  INSERT INTO calls_to__func_potato_exct_nrmld_score
  VALUES (p_dog_cat_fish_id);
  RETURN 0;
END;
$$;

This is done in order to isolate the procedure from its dependencies, and to capture call to the procedures for the purpose of validating that the calls were made with the appropriate inputs. the overwritten procedures capture the called parameter values in a table, so that they can be asserted against in later parts of the test. When tests are written this way, piggly apparently is unable to accurately capture the fact that the call:

PERFORM cat.func_potato_exct_nrmld_score(p_dog_cat_fish_id);

was actually made. Not sure how easy this will be to fix.

kputnam commented 6 years ago

Hmm, that might be tricky. What does your coverage report show exactly, and what did you hope it would show instead? I'm guessing it doesn't annotate the line which says PERFORM ... with any warning, but it simply doesn't have any reported coverage for func_potato_exct_nrmld_score?

On initialization, piggly replaces func_potato_exct_nrmld_score with another procedure that emits a RAISE WARNING 'hi this is func_potato_exct_nrmld_score' kind of message that's used to let piggly know that function started (and more messages throughout to signal which parts executed). You can see the instrumented code in cache/Trace/[md5]/code. Since you have stubbed that procedure out with one that doesn't emit coverage messages, you won't gain any coverage by calling the stub.

I think you can still get coverage information if you were to swap the stub back with the instrumented version. That might be tricky, but look at the SQL in lib/piggly/dumper/reified_procedure.rb and you may be able to modify that to save the code before you stub it out, then restore the code afterward (tricky, but plausible). You could do this either directly in PL/pgSQL or in another host language that your test driver will execute before/after running the test.

The other thing you could try is running your tests for the helper functions first (ones which don't need to stub out any calls) to get the coverage stats on them, and then execute the tests for functions which need to stub those helper functions out. So just running your tests in a particular order might avoid this. You might have circular dependencies or other complications that make this impossible though.

kputnam commented 6 years ago

To make my first suggestion more concrete, you could try something like this to save the source code before you replace it with a stub:

CREATE OR REPLACE FUNCTION save_source(fn regprocedure) RETURNS text AS $$
BEGIN
    -- SELECT pro.prosrc
    -- WHERE pro.oid = $1
    -- save this source in a temp table, or return it to the caller, or something
END;
$$ LANGUAGE plpgsql;

Then something like this to restore the source code, whether it's the version created by piggly or not, whatever it used to be before you stubbed it out:

CREATE OR REPLACE FUNCTION restore_source(fn regprocedure) RETURNS void AS $$
DECLARE
    source text := ... -- call to retrieve from temp table, or take it as a parameter to this proc?
BEGIN
    UPDATE pg_proc SET prosrc = source WHERE oid = $1;
END;
$$ LANGUAGE plpgsql;

Then your tests would look something like:

save_source('func_potato_exct_nrmld_score(integer)'::regprocedure);
-- define stub func_potato_exct_nrmld_score
-- execute function under test
restore_source('func_potato_exct_nrmld_score(integer)'::regprocedure);
kputnam commented 6 years ago

I updated the above comment a few times as I worked out more details of how this would actually work. It seems pretty plausible, you'd only need to decide if you want to store/load the source code in a temp table or to return it as text from save_source and take it as a paramater to restore_source.

born-in-brooklyn commented 6 years ago

interesting. so, to answer your first question, I'm simply getting 'never evaluated'. I get that the re-written procedure won't have the piggly RAISE WARNING stuff. by itself that's really not an issue. I simply wanted the coverage report to show that the PERFORM line was called. Its not important to test the actual procedure here, since that procedure is tested separately, in isolation. basically, I'm trying to do a unit test best practice my subbing out dependencies. If I wrote code to copy the piggly RAISE WARNING calls into the rewritten proc, piggly might erroneously report coverage on the dependent procedure, which is misleading. as far as restoring the old version of the procedure, all this work is done in a transaction, so rollback takes care of that.

kputnam commented 6 years ago

Ok, that all makes sense. I don't think PERFORM or EXECUTE are tracked individually, so the 'never evaluated' message probably applies to the entire block of code that your PERFORM statement belongs to. Maybe a BEGIN/END or other block of code that surrounds it.

You could look in piggly/cache/Trace/*/code for the instrumented version of your procedure under test to see where the perform public.piggly_branch($PIGGLY$696705f9d82d8299$PIGGLY$); instrumentation is. The identifier corresponds to the hyperlink '#T696705f9d82d8299' in the report, so you can find the code that way.

kputnam commented 6 years ago

as far as restoring the old version of the procedure, all this work is done in a transaction, so rollback takes care of that.

Ha, that's a much smarter approach. Sometimes not thinking about this stuff for a long time leads to ridiculous solutions like mine :-)