Skip to content

Remove stale xfail on schema-qualified INSERT parseutils test - #1625

Open
pacocartones wants to merge 2 commits into
dbcli:mainfrom
pacocartones:remove-stale-xfail-insert-schema
Open

Remove stale xfail on schema-qualified INSERT parseutils test#1625
pacocartones wants to merge 2 commits into
dbcli:mainfrom
pacocartones:remove-stale-xfail-insert-schema

Conversation

@pacocartones

Copy link
Copy Markdown

Description

test_simple_insert_single_table_schema_qualified in
tests/parseutils/test_parseutils.py was marked @pytest.mark.xfail back when an
older sqlparse mislabeled schema-qualified INSERT statements. That has long
been fixed. The test now passes across the whole supported range
(sqlparse >=0.3.0,<0.7 — I checked 0.3.0 and 0.6.0), so the marker only ever
produced an XPASS.

Since xfail_strict isn't set, that XPASS was silent, which means the assertion
wasn't actually guarding extract_tables anymore. This just drops the decorator
so the test does its job again. No production code changes.

Verified locally: the file passes repeatedly on both sqlparse 0.3.0 and 0.6.0,
and ruff format/ruff check are clean.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install).
  • I verified that my changes work as expected (this may include manually testing them in your local environment, or in other available environments). Cross this out if not relevant (for example, if you're making a documentation change).
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

test_simple_insert_single_table_schema_qualified was marked xfail for an
old sqlparse that mislabeled schema-qualified INSERT statements. That is
long fixed: the test passes across the supported range (sqlparse 0.3.0
through 0.6.x), so the marker only produced an XPASS. Because xfail_strict
is not set, that XPASS was silent and the assertion never gated anything.

Drop the decorator so the test guards extract_tables again.
Comment thread changelog.rst
``test_simple_insert_single_table_schema_qualified``. It was marked for an old
``sqlparse`` that mislabeled schema-qualified ``INSERT``; the test now passes
across the supported ``sqlparse`` range (0.3.0 to 0.6.x), so the marker only
hid a passing test (an unreported XPASS, since ``xfail_strict`` is not set).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove this. The changelog is for users of pgcli, who are unlikely to feel concerned and interested by this change.

Also, I am not sure that this warrants an addition in AUTHORS. Unless you really want your name there, of course...

@dbaty

dbaty commented Aug 28, 2026

Copy link
Copy Markdown
Member

Also, there is another xfail on tests.test_sqlcompletion.test_sub_select_multiple_col_name_completion(). It's been added by 4e86201 in 2015 (without explanation, unfortunately). Care to check if it's still needed? :)

dbaty asked whether the xfail on test_sub_select_multiple_col_name_completion
(added in 4e86201 in 2015, without explanation) is still needed. It is:
removing the marker makes the test fail on sqlparse 0.6.0. Parsing the
incomplete 'SELECT a, FROM abc' still treats the token before the trailing
comma ('a') as a table, so the Column suggestion carries table_refs for both
'a' and 'abc' instead of just 'abc'.

Keep the marker but give it a reason so the 2015 gap dbaty flagged is closed:
future readers see why the failure is expected instead of an undocumented
decorator.
@pacocartones

Copy link
Copy Markdown
Author

Good catch — thanks for pointing at 4e86201. I checked empirically: it's still needed. Dropping the marker makes test_sub_select_multiple_col_name_completion fail on sqlparse 0.6.0 — parsing the incomplete SELECT a, FROM abc still treats the token before the trailing comma (a) as a table, so the Column suggestion comes back with table_refs for both a and abc instead of just abc (and last_keyword differs too). So rather than remove it, I've added a reason= to the marker documenting the parser limitation — that closes the 2015 "no explanation" gap while keeping the expected-failure recorded. Pushed to this branch; ready for another look when you have a moment.

@pacocartones
pacocartones requested a review from dbaty September 4, 2026 01:34
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.

2 participants