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

Expand BlackboxPyTealer in an attempt to simplify dryrun request generation #335

Merged
merged 8 commits into from
May 12, 2022

Conversation

michaeldiamant
Copy link
Contributor

Optionally proposes a change to #322 that may simplify blackbox test setup.

While reviewing #322, it seemed to me that it's common to heavily reference BlackboxPyTealer when constructing a Graviton dryrun request. To simplify construction, the PR proposes moving construction into BlackboxPyTealer.

If the idea is worth keeping, then other DryRunInspector method ought to be provided.

Aside: I originally attempted to implement the PR by exposing a partially applied function in order to minimize parameter duplication.

  • However, due to my lack of Python understanding and/or available tooling, adding a class method as shown seems to be path of least resistance.
  • A disadvantage of the PR's approach is that it hides DryRunInspector. I can see downsides to adding another layer, which is why I present the PR optionally.

@tzaffi
Copy link
Contributor

tzaffi commented May 11, 2022

Have you looked at functools.partial ?

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

This is clearly an improvement, thank you! Since you're touching this code I also recommend you consider the following:

  1. the minor python 3.10 typing conventions I suggest
  2. I would be supportive of a clever use of functools.partials. I'm curious how this would look like.
  3. Now that BlackboxPyTealer can also run dry-runs, I think the suffix PyTealer is somewhat misleading. Consider a better? name such as SubroutineRunner or BlacboxTester or you can probably come up with something better. (make sure to announce it on slack's #christening channel)

@michaeldiamant
Copy link
Contributor Author

As of 7f5f526, I've made these changes per PR feedback:

  • Rename from BlackboxPyTealer to PyTealDryRunExecutor. I'm not strongly tied to the proposed name. I am also OK to keep the existing name.
  • Replace existing DryRunExecutor.dryrun_on_*_sequence invocations with PyTealDryRunExecutor.dryrun_on_sequence. In a child PR, I'll provide updates for other usages in case we want to limit changes.

Note - The build issue stems from the parent PR. I'll update the PR once the parent branch issue is resolved.

@tzaffi tzaffi merged commit 47db977 into blackbox-abi May 12, 2022
@tzaffi tzaffi deleted the blackbox-abi_pytealer_api branch May 12, 2022 21:33
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.

2 participants