[2/8] Use `assert(Not)Regex` instead of `assertTrue` or `assertFalse` in tests by Flamefire · Pull Request #5241 · easybuilders/easybuild-framework

Flamefire · 2026-08-26T14:17:26Z

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.

… in tests 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.

akesandgren · 2026-09-11T10:17:49Z

            res = get_log_filename('foo', '1.2.3', date='19700101', timestamp='094651', add_salt=True)
-            regex = re.compile(os.path.join(tmpdir, r'easybuild-foo-1\.2\.3-19700101\.094651\.[a-zA-Z]{5}\.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-19700101\.094651\.[a-zA-Z]{5}\.log$'))


Why the lack of re.compile here compared to the others above?

Or perhaps the question should be why keep the re.compile above?

re.compile was required when flags were used. E.g. when ^$ should match line begin/end. I removed the remaining ones that were no longer required now

akesandgren · 2026-09-11T10:26:45Z

-        for pattern in patterns:
-            regex = re.compile(pattern, re.M)
-            self.assertFalse(regex.search(desc), "Pattern '%s' not found in: %s" % (regex.pattern, desc))
+        self.assert_multi_regex(patterns, desc, assert_true=False)


Hmmm didn't think of this in the 1/8 PR... the assert_true is a weird name, just "assert" would make more sense, at least to me. (or at least something else than assert_true...)

I think this existed before. How about assert_match?

that feels better at least...

Done. Can you merge this soon so I can update the others before they fail post-merge?

akesandgren · 2026-09-11T11:39:44Z

@@ -255,7 +255,7 @@ def test_package(self):
        self.assertTrue(os.path.isfile(pkgfile))
        pkgtxt = read_file(pkgfile)
        self.assertRegex(pkgtxt, r"""DESCRIPTION:.*`backticks'.*""")
-        self.assertRegex(pkgtxt, re.compile(r"""DESCRIPTION:.*\nand newlines""", re.MULTILINE))
+        self.assertRegex(pkgtxt, r"""DESCRIPTION:.*\nand newlines""")


Don't these two actually need MULTILINE? the first one does have a "$" in the middle of the regex at least and the second one includes newline in the pattern

The first one? r"""DESCRIPTION:.*backticks'.*"""has no$`

MULTILINE only affects ^ and $ to match line start/end, so a \n always matches a newline

akesandgren · 2026-09-11T12:02:34Z

-        no_logfiles_regex = re.compile(r'STARTCONTENTS.*\.(log|md)$.*ENDCONTENTS', re.DOTALL | re.MULTILINE)
-        res = no_logfiles_regex.search(pkgtxt)
-        self.assertFalse(res, "Pattern not '%s' found in: %s" % (no_logfiles_regex.pattern, pkgtxt))
+        self.assertNotRegex(pkgtxt, re.compile(r'STARTCONTENTS.*\.(log|md)$.*ENDCONTENTS', re.DOTALL | re.M))


Sorry, this is the one I meant that has $ in the middle

Ok, I had kept the re.M there.

Flamefire · 2026-09-11T12:23:33Z

I pulled in another change to assert_multi_regex from one of the other PRs to avoid changing that multiple times.
It looks like for multiple regexes we usually want the multiline feature and we had used it as such

However IMO this is not a safe default: With re.M it may have additional matches, which is ok for assert_match=False but for assert_match=True I'd say it would be better to be explicit when we need it.
Maybe we should have that as default: multi_line = not assert_match

There is at least 1 check that is supposed to match at the beginning/end of the string, not each line.

akesandgren · 2026-09-14T05:07:09Z

            pattern = r"\s*extensions\("

-        self.assertFalse(re.search(pattern, desc), "No extensions found in: %s" % desc)
+        self.assertNotRegex(pattern, desc, re)


Still don't understand the "re" as third argument here. The third argument is the message string to be printed if you don't want the default.

Just a leftover. I thought I had fixed that, done now

akesandgren

LGTM

akesandgren

Looks even better now :-)

akesandgren · 2026-09-14T08:50:39Z

Going in, thanks @Flamefire!

Flamefire · 2026-09-15T09:54:42Z

@akesandgren Followup: #5285 as per my last comment.
Can you check that soon?

[2/8] Use assert(Not)Regex instead of assertTrue or assertFalse…

33ac366

… in tests 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 mentioned this pull request Aug 26, 2026

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

Closed

boegel added the tests label Sep 4, 2026

akesandgren reviewed Sep 11, 2026

View reviewed changes

Comment thread test/framework/module_generator.py Outdated

akesandgren reviewed Sep 11, 2026

View reviewed changes

Flamefire added 3 commits September 11, 2026 13:18

Merge branch 'develop' into assertregex-2

ee60934

Remove superflous re.compile calls

fb82e2d

Rename assert_true to assert_match

ede2655

akesandgren reviewed Sep 11, 2026

View reviewed changes

Change flags parameter to boolean multi_line

1a0dddd

There is at least 1 check that is supposed to match at the beginning/end of the string, not each line.

Flamefire force-pushed the assertregex-2 branch from 7097c20 to 1a0dddd Compare September 11, 2026 13:42

akesandgren reviewed Sep 14, 2026

View reviewed changes

Remove stray re argument

f8e0861

akesandgren previously approved these changes Sep 14, 2026

View reviewed changes

akesandgren added this to the next release (5.4.1) milestone Sep 14, 2026

Make typehint Python 3.6 compatible

3151130

Flamefire dismissed akesandgren’s stale review via 3151130 September 14, 2026 08:14

akesandgren approved these changes Sep 14, 2026

View reviewed changes

akesandgren merged commit 31da57d into easybuilders:develop Sep 14, 2026
41 checks passed

Flamefire deleted the assertregex-2 branch September 14, 2026 10:28

Conversation

Flamefire commented Aug 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamefire Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamefire commented Sep 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akesandgren left a comment

Choose a reason for hiding this comment

Uh oh!

akesandgren left a comment

Choose a reason for hiding this comment

Uh oh!

akesandgren commented Sep 14, 2026

Uh oh!

Uh oh!

Flamefire commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Flamefire Sep 11, 2026 •

edited

Loading