Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port more tests from TestBase to RuleRunner #10704

Merged
merged 2 commits into from Sep 1, 2020

Conversation

Eric-Arellano
Copy link
Contributor

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

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Sorry for the diff size. I marked where there is anything of interest. This was a pretty routine change facilitated by PyCharm + find and replace.

@@ -3,192 +3,208 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now use 3 distinct RuleRunners rather than 1 TestBase. This results in tighter rule graphs.

@@ -113,4 +114,4 @@ async def setup_pex_cli_process(


def rules():
return [*collect_rules(), *pex_environment.rules()]
return [*collect_rules(), *external_tool.rules(), *pex_environment.rules()]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes test setup far more ergonomic.

@@ -262,4 +264,4 @@ async def two_step_pex_from_targets(req: TwoStepPexFromTargetsRequest) -> TwoSte


def rules():
return collect_rules()
return (*collect_rules(), *pex_rules(), *python_sources_rules())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes test setup far more ergonomic.

@@ -20,7 +20,6 @@
from pants.engine.target import FieldSet, Sources, Target, Targets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These could have been unit tests the whole time. We don't use RuleRunner because there's no reason to.

@@ -73,6 +73,7 @@
from pants.engine.unions import UnionMembership, UnionRule, union
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use a couple distinct RuleRunners here, more than we had TestBases. This results in tighter graphs.

from pants.testutil.test_base import TestBase


class WorkspaceGoalSubsystem(GoalSubsystem):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class, and all the below rules / dataclasses, get inlined into the single unit test that use it. I figured this results in better namespacing.

class OptionsFingerprinterTest(TestBase):
def setUp(self) -> None:
super().setUp()
self.options_fingerprinter = OptionsFingerprinter()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of self.options_fingerprinter, we now create a new OptionsFingerprinter() every time we need it. Turns out, all the methods are really classmethods, even though they're marked as instance methods.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling fb46975 on Eric-Arellano:more-rule-runner into cf483ea on pantsbuild:master.

@Eric-Arellano Eric-Arellano merged commit 02a86d8 into pantsbuild:master Sep 1, 2020
@Eric-Arellano Eric-Arellano deleted the more-rule-runner branch September 1, 2020 00:41
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.

None yet

3 participants