Add RuleRunner as a Pytest-style replacement to TestBase

[Eric-Arellano]

Problem

We want to be able to use Pytest-style tests, rather than unittest style tests. Pytest allows for nice features like parameterization and fixtures, including dozens of pre-built fixtures like caplog (capture logging).

For our TestBase-style tests, we need a wrapper around a SchedulerSession in order to make synchronous calls to the engine. We also need to set up a temporary build root.

For isolation between tests, we must be careful to invalidate every test, such as using a new build root every individual test.

Solution

Add RuleRunner, which is a dataclass around all the config necessary to create a BuildConfiguration and SchedulerSession. This dataclass exposes the methods request_product() and run_goal_rule(), along with utilities like add_to_build_file().

Each individual test should create a new instance of a RuleRunner. This is important for isolation between tests.

Conventionally, this will be done with a Pytest fixture, which allows us to set up common config (e.g. rules and target types) for multiple tests but to still get a distinct RuleRunner instance for each test.

@pytest.fixture
def rule_runner() -> RuleRunner:
    return RuleRunner(rules=filedeps.rules(), target_types=[MockTarget, ProtobufLibrary])

def test_no_target(rule_runner: RuleRunner) -> None:
    rule_runner.create_file(...)
    rule_runner.request_product(..)

Users can also create the RuleRunner inline.

If there are multiple different configurations in a test file, the user may set up multiple different Pytest fixtures.

@pytest.fixture
def target_adaptor_rule_runner() -> RuleRunner:
    return RuleRunner(
        rules=[QueryRule(TargetAdaptor, (Address, OptionsBootstrapper))], target_types=[MockTgt]
    )

...

@pytest.fixture
def address_specs_rule_runner() -> RuleRunner:
    return RuleRunner(
        rules=[QueryRule(AddressesWithOrigins, (AddressSpecs, OptionsBootstrapper))],
        target_types=[MockTgt],
    )

Result

We can always use Pytest style tests now in Pants. We deprecate TestBase to be removed in 2.1.0.dev0.

Performance benchmark

We used to only create one SchedulerSession for the entire test class and to invalidate on every individual test. Now, we create a new SchedulerSession every single test. This ends up having a negligible performance impact.

Before:

multitime -n 10 ./pants --no-pantsd test --force src/python/pants/core/util_rules/filter_empty_sources_test.py
1: ./pants --no-pantsd test --force src/python/pants/core/util_rules/filter_empty_sources_test.py
            Mean        Std.Dev.    Min         Median      Max
real        9.269       0.135       8.976       9.286       9.468
user        9.226       0.143       8.992       9.201       9.498
sys         6.960       0.133       6.768       6.980       7.187

After:

multitime -n 10 ./pants --no-pantsd test --force src/python/pants/core/util_rules/filter_empty_sources_test.py
            Mean        Std.Dev.    Min         Median      Max
real        9.347       0.125       9.172       9.339       9.666
user        9.332       0.139       9.189       9.299       9.726
sys         7.083       0.123       6.853       7.096       7.359

Risk: breaking test isolation

It is possible for a user to use a single RuleRunner for multiple tests, e.g. one instance for the entire test class. This will reduce isolation between tests.

We mitigate this risk by only ever documenting how you should properly use RuleRunner, along with adding a warning to https://www.pantsbuild.org/v2.0/docs/rules-api-testing about this risk.

Unsolved issue: rule registration

It is still clunky to register the rules you need for your test to pass. This should still be solved, but it's left for a followup.

[ci skip-rust]
[ci skip-build-wheels]

[Eric-Arellano]

I marked which files have meaningful changes. All other files are routine rewrites to Pytest style.

[Eric-Arellano]

This file has meaningful changes because we now use parametrization. In fact, this resolves a TODO Benjy added a few months ago to enable Pytest style tests!

[Eric-Arellano]

This file has meaningful changes. We used to have one TestBase for multiple distinct tests - we used a single config even though they were very different tests.

Now, we use a mix of inlined RuleRunners + 2 Pytest fixtures so that each test has the tightest possible graph.

[Eric-Arellano]

This file has meaningful changes because we now use parametrization.

[Eric-Arellano]

There wasn't any need to use TestBase in the first place for these 2 tests.

[Eric-Arellano]

We need to still emulate isolated_local_store, which is used in fs_test.py to avoid using the global lmdb_store. Saving for a followup.

[Eric-Arellano]

Here and below is basically copy pasta from TestBase.

[Eric-Arellano]

Note that we have no way of registering these clean up methods:

pants/src/python/pants/testutil/test_base.py

Lines 243 to 258 in 2fc8a42

	def setUp(self):
	"""
	:API: public
	"""
	super().setUp()
	# Avoid resetting the Runtracker here, as that is specific to fork'd process cleanup.
	clean_global_runtime_state(reset_subsystem=True)
	self.addCleanup(self._reset_engine)
	safe_mkdir(self.build_root, clean=True)
	safe_mkdir(self.pants_workdir)
	self.addCleanup(safe_rmtree, self.build_root)
	BuildRoot().path = self.build_root
	self.addCleanup(BuildRoot().reset)

I don't think we need them, though. The tmpdir for build_root should be automatically cleaned. BuildRoot().path will get reset the next time that a new RuleRunner is created, and it can't impact outside of the test file thanks to v2's isolation.

[benjyw]

That sounds right to me.

[coveralls]

Coverage remained the same at 0.0% when pulling 8ec84ba on Eric-Arellano:rule-runner into 2fc8a42 on pantsbuild:master.

[benjyw]

I love it!

[benjyw]

👍

[benjyw]

That sounds right to me.