Skip to content

Use assert(Not)Regex instead of assertTrue or assertFalse in tests - #5188

Closed
Flamefire wants to merge 1 commit into
easybuilders:developfrom
Flamefire:assertregex
Closed

Flamefire wants to merge 1 commit into
easybuilders:developfrom
Flamefire:assertregex

Conversation

@Flamefire

@Flamefire Flamefire commented May 5, 2026 •

Copy link
Copy Markdown
Contributor

This PR is now split into smaller PRs (<= 5 modules, <= ~100 lines):

This makes the tests shorter and hence easier to read, avoids
re.compile calls and manual failure message composing.
In some cases assert_multi_regex could be used to avoid the explicit loops.

While doing that I put the re.compile into the assertion instead of assigning to a variable and remove the call when there are no flags required as assertRegex accepts strings or compiled expressions

Examples:

-        regex = re.compile(os.path.join(tmpdir, r'easybuild-foo-1\.2\.3-[0-9]{8}\.[0-9]{6}\.log$'))
-        self.assertTrue(regex.match(res), "Pattern '%s' matches '%s'" % (regex.pattern, res))
+        self.assertRegex(res, os.path.join(tmpdir, r'easybuild-foo-1\.2\.3-[0-9]{8}\.[0-9]{6}\.log$'))

-        regex = re.compile(r"^eb toy-0.0.eb --robot --debug -l", re.M)
-        self.assertTrue(regex.search(txt), "Pattern '%s' should be found in: %s" % (regex.pattern, txt))
+        self.assertRegex(txt, re.compile(r"^eb toy-0.0.eb --robot --debug -l", re.M))

-            fail_msg = "Pattern '%s' should be found in: %s" % (guarded_load_regex.pattern, modtxt)
-            self.assertTrue(guarded_load_regex.search(modtxt), fail_msg)
-            fail_msg = "Pattern '%s' should not be found in: %s" % (recursive_unload_regex.pattern, modtxt)
-            self.assertFalse(recursive_unload_regex.search(modtxt), fail_msg)
+            self.assertRegex(modtxt, guarded_load_regex)
+            self.assertNotRegex(modtxt, recursive_unload_regex)

Continuation of #4205 , #4170, #5134 to remove usage of assertTrue & assertFalse as for almost everything there is a better alternative

Also regex assertions are over-used. In some places assertIn or even assertEqual could be used which is clearer and faster.
However the replacements done here were mostly regex-based with lot's of manual inspection to find good patterns, and finding those where assertIn would work is even more difficult.

@Flamefire
Flamefire force-pushed the assertregex branch 2 times, most recently from a4574ae to 3ffb7a9 Compare May 5, 2026 13:44
@boegel boegel added the tests label Jun 16, 2026
@boegel boegel added this to the release after 5.3.1 milestone Jun 16, 2026
@boegel

boegel commented Jun 16, 2026

Copy link
Copy Markdown
Member

@Flamefire merge conflicts to resolve here...

@Flamefire
Flamefire force-pushed the assertregex branch 2 times, most recently from 77d9d21 to 7e75f03 Compare June 18, 2026 11:45
@Flamefire

Copy link
Copy Markdown
Contributor Author

Rebased

@boegel

boegel commented Aug 6, 2026

Copy link
Copy Markdown
Member

merge conflicts again...

As with #5223, I think it would help to break this into smaller PRs, which are easier to review/merge, and less likely to get blocked by merge conflicts

@Flamefire

Copy link
Copy Markdown
Contributor Author

This is a search&replace (using regex) which I think is better to do once, especially as it is only test code so mistakes would a) turn up in CI and b) are not critical to users. So I would not recommend multiple PRs but just do some quick spot checking.

If you still want separate PRs: How small do you want them? More PRs is more work but quicker to review.

@boegel

boegel commented Aug 26, 2026

Copy link
Copy Markdown
Member

This is a search&replace (using regex) which I think is better to do once, especially as it is only test code so mistakes would a) turn up in CI and b) are not critical to users. So I would not recommend multiple PRs but just do some quick spot checking.

If you still want separate PRs: How small do you want them? More PRs is more work but quicker to review.

Point taken. It's just trouble to get this reviewed, since it keeps "bitrotting" in between times we manage to take another look at it, and leading to merge conflicts (as is again the case now).

Naively I would say one PR per test module, but that's perhaps a step too far (that would be 25 tiny PRs).
How about PRs that change no more than say 100-ish lines, and only touch a handful of test modules (5-ish)?

@Flamefire

Copy link
Copy Markdown
Contributor Author

How about PRs that change no more than say 100-ish lines, and only touch a handful of test modules (5-ish)?

Ok done, all PRs mentioned in the description of this one. I'd keep this open in case at some point someone likes to merge the remaining changes in one go

@Flamefire
Flamefire force-pushed the assertregex branch 3 times, most recently from c0c4dad to 1e74993 Compare September 16, 2026 11:00
@Flamefire
Flamefire force-pushed the assertregex branch 3 times, most recently from 188aa68 to e2c076e Compare September 22, 2026 16:39
This makes the tests shorter and hence easier to read, avoids
`re.compile` calls and manual failure message composing.
In some cases `assert_multi_regex` could be used to avoid the explicit loops.
@Flamefire

Copy link
Copy Markdown
Contributor Author

All merged, no new occurrences in develop. Thanks!

@Flamefire Flamefire closed this Sep 30, 2026
@Flamefire
Flamefire deleted the assertregex branch September 30, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants