Skip to content

feat: add file pytest test-file emitter#5

Open
codeneu-A15 wants to merge 1 commit into
matte97p:mainfrom
codeneu-A15:feat-pytest-emitter
Open

feat: add file pytest test-file emitter#5
codeneu-A15 wants to merge 1 commit into
matte97p:mainfrom
codeneu-A15:feat-pytest-emitter

Conversation

@codeneu-A15

Copy link
Copy Markdown

Closes #3

Hi @matte97p! Here is the pytest test-file emitter for this issue.

I have implemented the following things:

  • Created src/rlsgrid/emitters/pytest.py based on the pgTAP reference.
  • Implemented the templates for _lives, _throws, and _is_count_zero assertions.
  • Implemented cross-tenant data isolation tests.
  • Wired the new emitter into the gen pytest CLI command.

I tested it locally against the dummy blog database and it successfully generates the test_rls.py file with all the assertions passing.

@matte97p matte97p self-requested a review June 1, 2026 12:42

@matte97p matte97p left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @codeneu-A15, thanks for picking this up! The overall shape matches the pgtap reference and the CLI wiring is clean. Before we can merge, there are a few issues to work through — some are structural, so I'd rather flag them all up front than drip-feed review rounds.

Blockers (the emitted file won't run as-is)

  1. Claim setter produces invalid Python source. json.dumps(rendered) contains ", then gets embedded inside an outer f"db_client.execute(\"…'{json_str}'…\")" — the inner " closes the outer string. The generated test file won't parse. Same class of bug in the cross-tenant WHERE builder: f"\"{col}\" = \\'{val}\\'" produces "id" = \'…\' which breaks both the surrounding Python string and is invalid SQL (Postgres doesn't honor \' outside E-strings).
    → Fix: stop building SQL as interpolated strings inside emitted Python. Emit parameterized calls: db_client.execute("SELECT count(*) FROM {qualified} WHERE \"id\" = %s", (pk_val,)). Removes a whole class of escaping bugs.

  2. SET LOCAL ROLE '{cell.role}' is wrong syntax. SET ROLE expects an identifier (SET ROLE foo or SET ROLE "foo"), not a string literal. Postgres errors out. Use the same _quote_ident pattern pgtap uses.

  3. SET LOCAL outside a transaction is a no-op + warning. pgtap wraps everything in BEGIN; … ROLLBACK;. The pytest emitter delegates to the db_client fixture but doesn't document that it must be transactional per-test. Either require it explicitly in the fixture docstring, or use SET ROLE (session) + RESET ROLE in a try/finally.

  4. Identifier quoting is missing everywhere. qualified = f"{cell.schema}.{cell.table}" raw. Tables with mixed case / reserved words / dots break. Reuse _quote_qualified from pgtap.

Strong suggestions

  1. Extract shared helpers. _quote_ident, _quote_literal, _quote_qualified, _is_bypass_role, _BYPASS_ROLES, _conditional_cells_with_data should move to emitters/_common.py so both emitters import them. Right now you're importing a private _conditional_cells_with_data cross-module.

  2. Bypass roles aren't filtered on the base path. pgtap skips service_role/postgres/supabase_admin. The pytest emitter only filters them inside _conditional_cells_with_data. Base ALLOW/DENY tests for those roles will fail spuriously.

  3. UPDATE probe hardcodes id. UPDATE {qualified} SET id = id WHERE false — tables without an id column return 42703 column does not exist, not 42501. Use introspection.pk_of(schema, table) like pgtap does.

  4. pytest.raises(Exception, match='42501') is fragile. psycopg3 carries SQLSTATE on exc.diag.sqlstate, not always in str(exc). Plus Exception is too broad — a KeyError from the fixture would pass. Prefer:

    with pytest.raises(Exception) as exc_info:
        db_client.execute(stmt)
    assert getattr(exc_info.value, 'sqlstate', None) == '42501'

    Or catch psycopg.errors.InsufficientPrivilege directly if you want to pin the driver.

  5. RESET ROLE runs after the assertion. If assert count == 0 fails, the exception skips RESET ROLE and the role leaks into the next test sharing the connection. Wrap in try/finally, or rely on a transactional fixture that rolls back per-test.

  6. Add tests/test_pytest_emit.py. Mirror test_pgtap_emit.py — at minimum a golden test for the base path and one with a seed_state fixture covering the conditional path. That second one would have caught blocker #1.

Nits

  • import json as _json inside gen_pytest — move to module top, like the rest of cli.py.
  • # Simplified for template comment on the UPDATE probe → resolve via pk_of() (#7) and drop the marker.
  • Test function names use {role}_{op}_{table} — two schemas with the same table name produce duplicate def test_… names. Include the schema (sanitized) or use a hash suffix.

Happy to pair on any of this, especially #1 (the parameterized rewrite) and #5 (the helper extraction) since those are the structural calls. Once those two land the rest is mechanical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pytest/vitest test-file emitters

2 participants