Cleanups and Speedup for python unittests #13439

dcbaker · 2024-07-16T22:22:45Z

I initially set out to just do some long overdue maintenance of our unit tests, along way I cleaned up a number of issues that have been bugging me for a while:

use mock instead of try/finally blocks
use os.enivon.get so that some things don't have to be set to run
move a lot of constant setup to setUpClass instead of setUp, which means the fixtures get run less
use more of our helpers instead of open coding
make more use of UnitTest.addCleanup
actually clean up more often, and move the cleanup code closer to the producer.
Allow our cleanup of Meson calls to happen even if the call to Meson itself fails
cleanup some complicated logic
use better/faster types
use properties for rarely used dynamic attributes
Use tmpdir instead of source dir when we can reasonably assume that there will not be Anti-Virus, and allow Windows users to force the use of the tmpdir

For me the end result is a reduction in a full unit test run time by ~15%, and there are no stray files left in the source tree compared to the main branch.

I wont be surprised if there are a few regressions on Windows to clean up.

dcbaker · 2024-07-16T22:24:38Z

I've also reworded a few of the commit messages to fix typos, but I'll wait to re-push so I don't flood the CI

eli-schwartz · 2024-07-16T23:14:49Z

unittests/linuxliketests.py

-        if is_cygwin():
-            self.new_builddir_in_tempdir()


This commit message doesn't justify why it's okay to allow the umask tests to now run in the source tree by default.

Because it doesn't run on the source tree, and that has nothing to do with the source tree? The source tree is always, unconditionally copied into a new directory, the only thing that changes here is where the build directory is being placed.

This commit message doesn't justify why it's okay to allow the umask tests to now run in the meson.git source tree by default.

And the reason I'm mentioning this is because the comment in the now deleted function says cygwin requires forcing a tempdir for this small subset of tests, it doesn't say anything about the relationship between the directory with meson.build and the one with build.ninja

Okay, your comment confused me, I thought you were suggesting that the test is now configuring from the source tree, not that there were issues with the build directory being in the source tree.

I'm curious as to what the actual original issue with installing in the source dir was, and why it doesn't seem to be a problem now, looking at the code there was a CI regression on cygwin (when we were running cygwin on azure) from this, but it doesn't seem to happen now. I'm really confused

I don't know either! :D

If we think it's no longer relevant then I'm not opposed to deleting it, it's just the commit message should indicate we are doing so (and why).

Okay, I've added a paragraph about those changes to the commit message.

Interesting and interesting. This failure seems to be sporadic. I'm going to guess there's a race condition involved, but it sounds hard to solve, so I just more-or-less put the workaround back.

eli-schwartz · 2024-07-16T23:18:45Z

unittests/platformagnostictests.py

@@ -187,7 +187,7 @@ def test_validate_dirs(self):

        # Using parent as builddir should fail
        with self.subTest('parent directory'):
-            self.builddir = os.path.dirname(self.builddir)
+            self.builddir = self.common_test_dir


I wonder if the solution to this is maybe that the _build_dir_root should be a single base directory in $TMPDIR, and all temporary builddirs should be created inside it?

But that wont work, this test as always pointed as a parent of the source directory, it just did it in a more round about way.

I mean that this actually made me think that we should do this:

/tmp/meson-tests-temp-XXXX /tmp/meson-tests-temp-XXXX/tmpYYYYY /tmp/meson-tests-temp-XXXX/tmpZZZZZ

If something goes catastrophically wrong in exiting the test harness, it's only one directory to rm -rf manually. And os.path.dirname(self.builddir) would be /tmp/meson-tests-temp-XXXX which is equally under our control, rather than being /tmp (my assumption is you didn't want the # Using parent as builddir should fail test to suddenly start trying to configure in /tmp itself).

I actually tried pointing at /tmp initially and it works fine, the call to os.path.basename works because of the assumption that the build directory is inside the source tree.

It's really unfortunate we can't use pytest, because it has "session" level fixtures that would make this trivial. The best we can do with unittest (I think) is a per module directory, so you'd still end up with:

/tmp/meson-tests-allplatformtests-XXXX/ /tmp/meson-tests-windowstests-XXXX/ /tmp/meson-tests-machinefiletests-XXXX/

eli-schwartz · 2024-07-16T23:22:34Z

unittests/baseplatformtests.py

@@ -88,11 +88,11 @@ def setUpClass(cls) -> None:

        # Misc stuff
        if cls.backend is Backend.ninja:
-            cls.no_rebuild_stdout = ['ninja: no work to do.', 'samu: nothing to do']
+            cls.no_rebuild_stdout = frozenset({'ninja: no work to do.', 'samu: nothing to do'})


Is there a genuine worry that this is going to be modified? Invoking frozenset() seems interesting in a commit claiming to be speeding things up. ;)

In my benchmarks using frozenset was slightly faster than a normal set for doing the lookups, and was only slightly slower for creating than a regular set. Since the frozenset only gets created once per class that seems like a pretty good tradeoff. ~~If you're still concerned about that, this actually only gets used in allplatformtests.py, so we could move it to that class to speed things up even more.~~ edit: it also gets used in WindowsTest

I guess I just wasn't sure why a set isn't good enough, that we need a frozenset. :P

You say a frozenset is slightly faster but I was under the impression that they both use the same CPython implementation and that frozensets are just sets with the mutability methods removed and __hash__ added so that you can use mydict[frozenset({'one'})]. So I would be surprised if there was any performance difference that wasn't actually environment noise e.g. system load at the time the benchmark was done.

The real difference between the two is that sets have syntax for construction, whereas frozensets always incur a name lookup and function call at the time of creation.

Maybe I've just been doing too much C++ and Rust recently, and would rather have immutable than mutable types by default :D

dcbaker · 2024-07-17T16:30:20Z

Squashed and re-ordered a bit to address comments from IRC/Matrix. Attempting to fix the one test that is failing on Windows

unittests/linuxliketests.py

eli-schwartz · 2024-08-15T19:16:17Z

Partial review and partial merge of commits 85e9233...4b76aab

eli-schwartz

oops, late...

unittests/linuxliketests.py

eli-schwartz · 2024-08-20T17:00:31Z

unittests/platformagnostictests.py

+        with self.subTest('parent directory'):
+            self.builddir = os.path.dirname(self.builddir)


This seems nice. Just thought I'd note the commit message for this claims these are "uint tests"...

This moves the cleanup callback to the point the file is being created, which is more accurate, and doesn't require adding `if os.path.exists()` calls

Rather than relying on callers to cleanup after the helper.

To get better error messages

Stop thrashing users SSDs because Windows AV is broken. This also helps IDEs that try to keep up with the quickly created and destroyed files. Users on Windows and Cygwin who don't have catastrophically bad AV solutions can use the MESON_UNIT_TEST_FORCE_TMPDIR=1 environment variable to get better behavior.

Set lookups are faster for membership checks.

This avoids having to set them up in cases we don't use them, or override them in cases where we change the build directory

jpakkane · 2024-10-02T22:06:47Z

unittests/failuretests.py

@@ -268,15 +268,13 @@ def test_subproject_variables(self):
        '''
        tdir = os.path.join(self.unit_test_dir, '20 subproj dep variables')
        stray_file = os.path.join(tdir, 'subprojects/subsubproject.wrap')
-        if os.path.exists(stray_file):
-            windows_proof_rm(stray_file)
+        self.addCleanup(windows_proof_rm, stray_file)


This changes behaviour. The old code ensured that if the file was already there for whatever reason it would be deleted before the test was run.

Indeed, it's changing behavior. The current behavior is:

the test creates the file

the test runs, and makes no effort to delete the file

on re-run the test deletes the file
This is weird, and not something that we generally do.

This changes the behavior to match what basically all tests do, and what should happen:

the test creates the file

the test registers a callback to ensure that the file is cleaned up when the test exits, regardless of whether that is in error or in success

on re-run the test doesn't need to look for the file, because it doesn't exist.

We have tons of tests that make the assumption "files I create as part of my test process do no pre-exist", and will die horribly, or worse, fail silently if they do.

jpakkane · 2024-10-02T22:10:18Z

Other than the one change LGTM.

dcbaker requested a review from jpakkane as a code owner July 16, 2024 22:22

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch from a94fe1c to 2f66081 Compare July 16, 2024 22:51

eli-schwartz reviewed Jul 16, 2024

View reviewed changes

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch from 2f66081 to a9626a6 Compare July 17, 2024 16:28

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch 4 times, most recently from 099f255 to 8ad08d7 Compare July 17, 2024 18:51

eli-schwartz reviewed Jul 17, 2024

View reviewed changes

unittests/linuxliketests.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch from 8ad08d7 to 880b885 Compare July 17, 2024 19:23

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch 2 times, most recently from 9a803fa to 4c9587c Compare August 26, 2024 20:58

eli-schwartz reviewed Sep 13, 2024

View reviewed changes

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch from 4c9587c to 950d96d Compare September 13, 2024 16:45

dcbaker added 10 commits October 1, 2024 15:08

unittests: use the skip_if_not_language decorator

f41d9a0

unittests: use more of the copy_srcdir helper

5d3950c

unittests: use more Unittest.addCleanup instead of try/finally

7c071dd

This moves the cleanup callback to the point the file is being created, which is more accurate, and doesn't require adding `if os.path.exists()` calls

unittests: have helpers call addCleanup for created files internally

5f76db8

Rather than relying on callers to cleanup after the helper.

unittests: Use a lot more self.addCleanup for file cleanup

e07aed8

unittests: simplify/clarify skip conditions

7e1f3b8

unittests: split test_validate_dirs with subTest

bd391d6

To get better error messages

unittests: use a set to store the no_rebuild_stdout

5104a04

Set lookups are faster for membership checks.

unittests: use property for rarely used directories

6dce000

This avoids having to set them up in cases we don't use them, or override them in cases where we change the build directory

dcbaker force-pushed the submit/unittest-cleanups-and-speedups branch from 950d96d to 6dce000 Compare October 1, 2024 22:11

jpakkane reviewed Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanups and Speedup for python unittests #13439

Cleanups and Speedup for python unittests #13439

dcbaker commented Jul 16, 2024

dcbaker commented Jul 16, 2024

eli-schwartz Jul 16, 2024

dcbaker Jul 17, 2024

eli-schwartz Jul 17, 2024

eli-schwartz Jul 17, 2024

dcbaker Jul 17, 2024

eli-schwartz Jul 17, 2024

dcbaker Jul 17, 2024

dcbaker Jul 17, 2024

eli-schwartz Jul 16, 2024

dcbaker Jul 17, 2024 •

edited

Loading

eli-schwartz Jul 17, 2024

dcbaker Jul 17, 2024 •

edited

Loading

eli-schwartz Jul 16, 2024

dcbaker Jul 17, 2024 •

edited

Loading

eli-schwartz Jul 17, 2024 •

edited

Loading

dcbaker Jul 17, 2024

dcbaker commented Jul 17, 2024

eli-schwartz commented Aug 15, 2024

eli-schwartz left a comment

eli-schwartz Aug 20, 2024

jpakkane Oct 2, 2024

dcbaker Oct 3, 2024 •

edited

Loading

jpakkane commented Oct 2, 2024

		with self.subTest('parent directory'):
		self.builddir = os.path.dirname(self.builddir)

Cleanups and Speedup for python unittests #13439

Are you sure you want to change the base?

Cleanups and Speedup for python unittests #13439

Conversation

dcbaker commented Jul 16, 2024

dcbaker commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

eli-schwartz Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker commented Jul 17, 2024

eli-schwartz commented Aug 15, 2024

eli-schwartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

jpakkane commented Oct 2, 2024

dcbaker Jul 17, 2024 •

edited

Loading

dcbaker Jul 17, 2024 •

edited

Loading

dcbaker Jul 17, 2024 •

edited

Loading

eli-schwartz Jul 17, 2024 •

edited

Loading

dcbaker Oct 3, 2024 •

edited

Loading