Skip to content

Issue 7843 - SyntaxWarning in the test suite - #7856

Merged
progier389 merged 1 commit into
389ds:mainfrom
xhon-pelushi:issue-7843-tests-syntaxwarning
Sep 17, 2026
Merged

progier389 merged 1 commit into
389ds:mainfrom
xhon-pelushi:issue-7843-tests-syntaxwarning

Conversation

@xhon-pelushi

@xhon-pelushi xhon-pelushi commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Relates to #7843. This is the follow-up I offered in #7844.

#7843 is about one SyntaxWarning from lib389/lint.py. Compiling every .py in the tree finds 21 more, all in dirsrvtests, so pytest output stays noisy once that one is fixed:

sequence file
'\ ' acl/globalgroup_part2_test.py, acl/globalgroup_test.py
'\(' '\{' acl/syntax_test.py
'\(' '\_' ds_logs/ds_logs_test.py
'\=' fourwaymmr/fourwaymmr_test.py
'|' monitor/monitor_test.py
'\(' replication/acceptance_test.py, replication/virtual_attribute_replication_test.py, roles/basic_test.py, tickets/ticket48973_test.py
'\*' tickets/ticket49471_test.py

They evaluate to a backslash followed by the character, so the values are right today, but 3.12 warns on each and the sequences are slated to become errors later.

Not a blanket raw-string sweep

Most become raw strings, which is what the regular expressions, LDAP filters and DN escapes wanted anyway. Three could not, because they mix an invalid escape with a real one that raw would turn into two characters:

  • the roles and virtual-attribute assertions also contain \'. Marking them raw would leave a literal backslash before each quote and the assertion would stop matching, so the quotes are unescaped by hand — inside a double-quoted string they need no escape.
  • ticket48973_test.py also contains \", so the outer quotes are swapped to single ones.
  • syntax_test.py:118 is an f-string, f'(target = ldap:\\\{DEFAULT_SUFFIX})...'. That is a real \\ followed by an invalid \{, and the brace still interpolates, so the value has two backslashes and the suffix substituted. It becomes \\\\ with a plain {DEFAULT_SUFFIX}.

Verification

No test string changes value. Each file was compiled before and after and its string constants compared recursively:

globalgroup_part2_test.py               consts= 128  identical=True  warn 3->0
globalgroup_test.py                     consts= 125  identical=True  warn 3->0
syntax_test.py                          consts=  93  identical=True  warn 2->0
ds_logs_test.py                         consts= 448  identical=True  warn 2->0
fourwaymmr_test.py                      consts=  87  identical=True  warn 1->0
monitor_test.py                         consts=  84  identical=True  warn 1->0
acceptance_test.py                      consts= 219  identical=True  warn 1->0
virtual_attribute_replication_test.py   consts=  61  identical=True  warn 2->0
basic_test.py                           consts= 186  identical=True  warn 4->0
ticket48973_test.py                     consts=  63  identical=True  warn 1->0
ticket49471_test.py                     consts=  11  identical=True  warn 1->0

1505 constants, all byte identical, total warnings 21 -> 0

That check is the reason I am comfortable sending this without a server: it does not matter whether I judged each literal correctly by eye, because any literal whose value moved would have shown up as a differing constant. After this, src/lib389/lib389/lint.py is the only invalid escape left in the tree, and #7844 covers it.

I have not run the suite — these need a live instance — so please shout if any of the rewritten assertions look wrong to you in context.

Summary by Sourcery

Remove invalid escape sequences from directory server tests to keep the test suite warning-free on newer Python versions.

Enhancements:

  • Eliminate invalid escape-sequence warnings from the dirsrvtests suite while preserving all string values and test behavior.

Tests:

  • Verify that all affected test files compile without warnings and that their string constants remain byte-identical.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Bug description:
	389ds#7843 reports a SyntaxWarning from lib389/lint.py showing up in test
	output. Compiling every .py in the tree finds 21 more, all in
	dirsrvtests, so pytest output stays noisy once that one is fixed:

	    '\ '  globalgroup_part2_test.py, globalgroup_test.py
	    '\(' '\{'  syntax_test.py
	    '\(' '\_'  ds_logs_test.py
	    '\='  fourwaymmr_test.py
	    '\|'  monitor_test.py
	    '\('  acceptance_test.py
	    '\('  virtual_attribute_replication_test.py
	    '\('  basic_test.py
	    '\('  ticket48973_test.py
	    '\*'  ticket49471_test.py

	These evaluate to a backslash followed by the character, so the
	values are correct today, but Python 3.12 warns on each and the
	sequences are slated to become errors in a later release.

Fix description:
	Mark the literals raw, which is what regular expressions, LDAP
	filters and DN escapes want anyway.

	Three of them could not simply be marked raw, because they mix an
	invalid escape with a real one that raw would turn into two
	characters:

	- the roles and virtual attribute assertions also contain \', so
	  the quotes are unescaped by hand; inside a double quoted string
	  they need no escape
	- ticket48973_test.py also contains \", so the outer quotes are
	  swapped to single
	- syntax_test.py:118 is an f-string where \\\{ is a real \\
	  followed by an invalid \{, and the brace still interpolates; it
	  becomes \\\\ with a plain {DEFAULT_SUFFIX}

	No test string changes. Each file was compiled before and after and
	its string constants compared recursively: 1505 constants across the
	11 files, all byte identical, with the warning count going from 21
	to 0.

	lib389/lint.py is left alone here, it is fixed in 389ds#7844.

Relates: 389ds#7843
@xhon-pelushi
xhon-pelushi force-pushed the issue-7843-tests-syntaxwarning branch from 3eb1735 to 393c432 Compare September 16, 2026 01:07
@packit-as-a-service

Copy link
Copy Markdown

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo dnf install -y 'dnf*-command(copr)'
  • dnf copr enable packit/389ds-389-ds-base-7856
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@progier389

Copy link
Copy Markdown
Contributor

Changes looks good. Waiting for the test result before approving and merging this PR

@xhon-pelushi

Copy link
Copy Markdown
Contributor Author

Thanks @progier389. On waiting for the test result — the current run is 242 green and 4 red, and
none of the 4 can be caused by this PR. Posting the comparison in case it saves you the dig; you
know this CI far better than I do, so treat it as data rather than a conclusion.

What this PR touches. Only test files, in these suites:

acl, ds_logs, fourwaymmr, monitor, replication, roles, tickets

What is failing. npm-audit-ci, LMDB Test (tls), BDB Test (tls), LMDB Test (webui) —
dirsrvtests/tests/suites/tls/ and .../webui/, neither of which this PR goes near.

And the same jobs are red on everyone's PRs right now:

PR author failing
#7839 mreynolds389 npm-audit-ci, BDB Test (password), LMDB/BDB Test (tls), LMDB/BDB Test (webui), + 6 rpm-build
#7846 jchapma npm-audit-ci, LMDB Test (plugins), BDB Test (webui)
#7849 drewrukin npm-audit-ci, LMDB Test (logging), LMDB/BDB Test (tls)
#7836 ushevchenko npm-audit-ci, BDB Test (clu), LMDB/BDB Test (tls), LMDB Test (webui)
#7856 this one npm-audit-ci, LMDB/BDB Test (tls), LMDB Test (webui)

npm-audit-ci audits the cockpit console's npm dependencies, so it cannot be affected by a change
to Python test files either way.

I am not asking you to merge over a red tick — if you would rather wait for a clean baseline that
is completely reasonable, and I am in no hurry. Just flagging that the tls/webui pair looks like it
will stay red regardless of this PR, so "wait for green" may not terminate on its own.

Happy to rebase onto current main if you would like a fresh run, or to leave it alone.

@progier389 progier389 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@progier389
progier389 merged commit 4849358 into 389ds:main Sep 17, 2026
242 of 246 checks passed
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