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

rptest: more typing 1 #15993

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

travisdowns
Copy link
Member

Add more PEP-484 typing to some rptest infra.

The start of a long journey.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

@travisdowns
Copy link
Member Author

travisdowns commented Jan 8, 2024

No runtime changes here, only additional type hints or modifications of existing type hints so it should be completely safe.

@@ -4240,10 +4240,10 @@ def estimate_bytes_written(self):
def make_redpanda_service(context: TestContext,
num_brokers: Optional[int],
*,
cloud_tier: Optional[str] = None,
cloud_tier: CloudTierName | None = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

The PEP-484 recommended approach is Type | None instead of Optional[Type].

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Jan 8, 2024

new failures in https://buildkite.com/redpanda/redpanda/builds/43562#018cea0a-f34a-4b31-857b-7aac95e23c5c:

"rptest.tests.recovery_mode_test.DisablingPartitionsTest.test_disable"

new failures in https://buildkite.com/redpanda/redpanda/builds/43562#018cea0a-f350-4899-bdfe-d6ce76ad1472:

"rptest.tests.cloud_storage_timing_stress_test.CloudStorageTimingStressTest.test_cloud_storage.cleanup_policy=delete"

Add more typing to some rptest infra.

The start of a long journey.
Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM on green

@andrewhsu
Copy link
Member

@travisdowns is this PR part of a desire to be more strict on python type hinting for all python in the repo?

if so, perhaps we should have a gha to run mypy:

mypy --ignore-missing-imports --disallow-untyped-defs . # or some list of files that grows as more have type hinting added

@dotnwat dotnwat merged commit a68485f into redpanda-data:dev Jan 9, 2024
17 checks passed
@travisdowns
Copy link
Member Author

@andrewhsu wrote:

@travisdowns is this PR part of a desire to be more strict on python type hinting for all python in the repo?

Yes, sort of, but focused on DT.

I think types are very useful for a relatively large thing like our DT suite. It has the highest value for the "frameworky" parts like the service classes and helper methods, since they are widely used and once types are added in there they tend to propagate around to the calling code and make more analysis possible.

if so, perhaps we should have a gha to run mypy:

Absolutely, that would be my longer-term desired if everyone is on board. A ways to go before most files run clean in mypy.

In the meantime it still provides a fair amount of value in IDEs etc.

In parallel I'm typing ducktape package code itself using stubs (pyi files) which is needed to get good results here since we use so many ducktape methods. I'll send some of that up in a PR at some point too.

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

4 participants